Simplify sample files required

[Wilfred]

Currently, adding a new sample requires all of:

the_file.php
the-file.php.hhconfig // empty
the-file.php.hhvm.expect // usually empty
the-file.php.no.auto.output // empty
the-file.php.typechecker.expect // usually "No errors!"

I propose that the_file.php should be sufficient. The others should only be necessary if they differ from the default. Most sample files don't/shouldn't need to show their output.

[fredemmott]

the-file.php.hhvm.expect // usually empty

This is, in general, an antipattern - but a common one.

Agreed on the rest. no.auto.output is not required, but is usually desired, but that should probably be the default behavior.

[fredemmott]

FYI changes to this set will require both changes to the 'fast' runner embedded here, and to hphp/test/run.php

[Wilfred]

This is, in general, an antipattern - but a common one.

Just to clarify: you're proposing that most files should output something, so we can check the output?

I was thinking a valid type check was usually sufficient. Otherwise many of our examples require a distracting <<__EntryPoint>> shim.

The alternative is we develop some kind of // BEGIN_INCLUDED_SAMPLE // END_INCLUDED_SAMPLE convention for code files. I'd really like this for simple cases like assignments or if statements, so I can only show the if statement but still have some checking.

[fredemmott]

Just to clarify: you're proposing that most files should output something, so we can check the output?

Yes, or at least it should clearly be an intentional decision to have a script with no output

The alternative is we develop some kind of // BEGIN_INCLUDED_SAMPLE // END_INCLUDED_SAMPLE convention for code files.

@jjergus recently added support for @example-start and @example-end :)

[jjergus]

Otherwise many of our examples require a distracting <<__EntryPoint>> shim.

I added some magic to automatically strip the common boilerplate (@example-start and @example-end can be used to override it but usually it does the right thing even without those; by "usually" I mean I'm parsing the code with regular expressions, what could go wrong).

https://github.com/hhvm/user-documentation/blob/c5c62ecb0f5862d371b16333a60e473dce00f5fb/src/markdown-extensions/ExampleIncludeBlock.php#L101

[lexidor]

Maybe an unwelcome suggestion, but the biggest problem I have with the current system is the amount of files it makes. Most of the are just clutter. Couldn't we put all these things in one file? For example a json file of sorts? We'd have to instruct hphp/test/run.php to decode the json file, but that isn't the hard part.

You lose the ability to use common command line operations like diff, xargs and friends this way though.

[jjergus]

It should be possible to get it down to 2 files per example I think, which seems reasonable to me.

It would be either

• the code (.hack) + expected output (maybe with different extension for output that should be shown in the guide vs output that should be omitted but still tested), test would asssume no typechecker errors
• the code + expected typechecker errors

[fredemmott]

Couldn't we put all these things in one file? For example a json file of sorts?

Pretty strongly against this:

• being compatible with the way Hack/HHVM internal tests are written is a big plus, especially with trying to get feature authors to write documentation
• a load of HHVM's built in tests were converted from PHP ones, which are single-file - it is much easier to update this format when things change, e.g. hhvm -c foo.ini foo.hack > foo.expect

[fredemmott]

the_file.php

This could/should be extracted by the build system from code blocks in the markdown:

• would improve developer experience
• when an example needs updating because of a breaking change, we should look at the surrounding text. I'm not good at actually remembering to do this... embedding would

Extracted files could contain a marker telling them how to regenerate them, and e.g. the hash of the .md file at the time they were generated

[fredemmott]

That said, at some point, we probably do want to end up with .php/.hack files on disk: the test run is much faster now that all the 'no errors' files are covered by the main hh_client run, rather than needing a distinct run per test.

[Wilfred]

8851baa2fc79bc4d2ebf62eb0a4784e5e81572bb ended up with 108 files, so we can clean this up once we have fewer required files :)

[jjergus]

I'm thinking of doing this along with #865. My current plan:

• rename everything from .php -> .hack
• the-file.hack.hhconfig -- make optional, only needed if non-empty
• require exactly one of the-file.hack.hhvm.expect or the-file.hack.no.auto.output or the-file.hack.typechecker.expect
  • .typechecker.expect should be provided iff other than "No errors!"; examples with errors should not be executed so .expect is irrelevant
    • or are there any cases where we would want to execute examples with errors?
  • the-file.php.hhvm.expect or the-file.php.no.auto.output should contain the expected output (which ideally should be non-empty, so that every example is properly tested), the file extension governs whether the output is rendered in the documentation or whether it's only used for testing.
    • Maybe rename .no.auto.output to something like .expect.no.auto.output?
    • Alternatively, should we just have .expect with some magic string (// @hide) used to govern if it should be rendered or just tested?

[fredemmott]

The no.auto.output files are unrelated to testing - they merely prevent the markdown processor from including the output in the produced documentation. Their presence should be entirely independent of expect

[jjergus]

That's how they work currently, but my proposal is to change that so that we always need just 1 file.

Current:

• .expect is required, .no.auto.output is optional
• .expect contains expected output, .no.auto.output is empty
• existence of .no.auto.output governs if rendered inside documentation

Proposed:

• either .expect or .no.auto.output should exist but not both
• either way, whichever file exists, it contains the expected output
• the filename (.expect vs .no.auto.output) governs if rendered inside documentation

[fredemmott]

• how do .expectf and .expectre fit in?
• you'll probably get pushback here: this requires making the HHVM test runner consider '.no.auto.output' a valid expect file, and that doesn't really seem appropriate

[fredemmott]

alternatives:

• kill .no.auto.output completely
• flip the default: Require a marker file on the rare occasions that embedded output is actually wanted
• always put output in an html5 <details>+<summary> pair that needs expanding (related to 'kill it completely)

[jjergus]

how do .expectf and .expectre fit in?

I think we want to require exactly one of all the alternatives: expect / expectf / expectre / typechecker.expect

you'll probably get pushback here: this requires making the HHVM test runner consider '.no.auto.output' a valid expect file, and that doesn't really seem appropriate

Oh OK if we need to preserve compatibility with that, we could do something like:

• Example with rendered output:
  • foo.hack
  • foo.hack.expect
• Example with hidden output:
  • foo.no.auto.output.hack (probably come up with something shorter)
  • foo.no.auto.output.hack.expect
• (we can also flip the default, foo.auto.output.hack vs foo.hack, I'll check which is more common)

[jjergus]

We already support a few magic substrings like that: https://github.com/hhvm/user-documentation/blob/master/tests/ExamplesTest.php#L21

[fredemmott]

Oh OK if we need to preserve compatibility with that, we could do something like:

We only use a custom thing for typechecker tests - we use run.php for all the runtime behavior at present. That should change, details in #863 though

[fredemmott]

(we can also flip the default, foo.auto.output.hack vs foo.hack, I'll check which is more common)

FWIW this was originally the other way around, and most are still from there - I think the current state was more "better to include it unneeded than not when it might be needed" rather than looking at normal, and I think that decision undervalued the downside of decreased readability when including them