Skip to content

Syntax examples #3085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Sep 27, 2024
Merged

Syntax examples #3085

merged 12 commits into from
Sep 27, 2024

Conversation

goldfirere
Copy link
Collaborator

@goldfirere goldfirere commented Sep 26, 2024

This PR expands the role of the pre-existing source_jane_syntax.ml.

It now tests that Pprintast correctly round-trips (as it used to). But it is also now an expect-test, thus serving as a convenient way to document our syntax.

Some of our syntax (like [@nontail]) is in the form of attributes. This conflicted with a feature of the round-trip test that checked that we had done jane-syntax encoding correctly. Given that it was hard to see a good way around this and that jane-syntax is on the way out, I just dropped the check.

In the course of preparing this PR, I found two places where Pprintast didn't round-trip, now fixed.

This PR additionally adds some automation that will remind us all to edit this file whenever we change the syntax of the language.

@ncik-roberts
Copy link
Contributor

ncik-roberts commented Sep 27, 2024

I see ocaml/testsuite/tests/parsetree/source_jane_street.ml as somewhat serving this purpose. The benefit is that we also test that pprintast round-trips against this file.

I think it's better if we keep just one of these files.

@goldfirere
Copy link
Collaborator Author

goldfirere commented Sep 27, 2024

(Written in a race with @ncik-roberts's comment above.)

@ccasin pointed out on Slack two truths:

  • We already have tests in language-extensions/pprintast_unconditional.ml that should, in theory, cover all of our new syntax.
  • Adding yet another place to forget to update tests is likely not a winning scenario.

The redundancy with pprintast_unconditional.ml is regrettable. But I still think this new file is helpful: it is much clearer to read than pprintast_unconditional.ml. I advocate keeping both.

About forgetting: this is a good point. So I have now added a workflow that will make a comment reminding you to update these tests whenever parser.mly is edited. You can see this in action at goldfirere#2

What do we think?

@goldfirere
Copy link
Collaborator Author

I agree that my new examples file is redundant with source_jane_street.ml. I will modify this now to use that file.

@goldfirere
Copy link
Collaborator Author

OK. Ready for review. Please re-read the PR description, as this has significantly changed.

@ncik-roberts ncik-roberts self-requested a review September 27, 2024 19:08
@goldfirere
Copy link
Collaborator Author

Fixing the broken test required making it a bit worse. Quick re-review from @ncik-roberts, please?

@ncik-roberts
Copy link
Contributor

Your edits to the test look reasonable. It does mean that test_ppx.ml won't catch issues like what originally motivated #2145, but that's fine: most such issues are related to attributes, and Jane Syntax is on its way out.

@ncik-roberts ncik-roberts merged commit a413898 into main Sep 27, 2024
16 checks passed
@ncik-roberts ncik-roberts deleted the rae/examples branch September 27, 2024 21:32
lukemaurer pushed a commit that referenced this pull request Oct 23, 2024
* Add syntax examples

* Add a little workflow to request examples for new syntax

* With permissions now

* Formatting. Avoid duplicates.

* Protect backticks

* Fix bug where we printed invalid syntax

* Fix bug in printing module strengthening

* Move examples into source_jane_street

* Update reminder script and add some commentary

* Remove redundant examples.ml

* Remove dead code from test

* Fix broken test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants