Fix top level extensions #2693
Merged
Fix top level extensions #2693
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…el-extensions * 'master' of github.com:/reasonml/reason: chore: update nix flake (reasonml#2692) chore(readme): clarify 3.9 is unreleased Fix ppat_constraint case (reasonml#2690)
davesnx
commented
Mar 6, 2023
SanderSpies
reviewed
Mar 6, 2023
| let last = match (List.rev structureItems) with | last::_ -> last | [] -> assert false in | ||
| let loc_start = first.pstr_loc.loc_start in | ||
| let loc_end = last.pstr_loc.loc_end in | ||
| let items = | ||
| groupAndPrint | ||
| ~xf:self#structure_item | ||
| ~xf:self#top_level_structure_item |
There was a problem hiding this comment.
Probably a bad name, but the logic is to check for a specific combination of AST such as: structure -> structure_item -> Pexp_extension
davesnx
force-pushed
the
Fix-top-level-extensions
branch
from
e9cbe7b
to
48e5ac9
Compare
anmonteiro
reviewed
Mar 7, 2023
| @@ -348,7 +348,7 @@ let () = { | |||
| something_else(); | |||
| }; | |||
|
|
|||
| [%bs.raw x => x]; | |||
There was a problem hiding this comment.
don't we also accept this form? I think it should keep being tested.
There was a problem hiding this comment.
Pushed back the test case, we do support them.
There was a problem hiding this comment.
It's still removed. did you forget to push?
There was a problem hiding this comment.
I added all of the cases in the cram tests. Let me add them in the formatTest as well
anmonteiro
reviewed
Mar 7, 2023
| method structure structureItems = | ||
| (* We don't have any way to know if an extension is placed at the top level by the parsetree |
There was a problem hiding this comment.
I thought we supported extensions at the toplevel with %, and that %%bs.raw was specifically a bucklescript thing.
There was a problem hiding this comment.
It is a BuckleScript thing, but we support both cases. Would be a nice idea to unify all of these cases with Melange. %bs.raw -> bs.raw_js_expr and %%bs.raw -> bs.raw_js or something like this.
There was a problem hiding this comment.
I don't get what needs to be unified?
There was a problem hiding this comment.
We could avoid having % and %% at the same time and support only %
…el-extensions * 'master' of github.com:/reasonml/reason: Install before importing deps Update package json and install esy normally Re-arrange esy install Reduce install esy time Use master branch instead of main Ignore _export from esy Rename refmt_test to test Add esy-ci and opam-ci Remove jbuild-ignore
anmonteiro
approved these changes
Mar 18, 2023
SanderSpies
approved these changes
Mar 18, 2023
davesnx
added a commit
to davesnx/reason
that referenced
this pull request
Mar 18, 2023
…mat-type-tests-to-cram * 'master' of github.com:/reasonml/reason: Fix top level extensions (reasonml#2693) Install before importing deps Update package json and install esy normally Re-arrange esy install Reduce install esy time Use master branch instead of main Ignore _export from esy Rename refmt_test to test Add esy-ci and opam-ci Remove jbuild-ignore
SanderSpies
added a commit
to SanderSpies/reason
that referenced
this pull request
Apr 18, 2023
* master: (38 commits) chore: remove old BS_NO_COMPILER_PATCH flag (reasonml#2710) Improve printing of modules types with one line inside (reasonml#2709) generate opam files with dune (reasonml#2704) Fix version on refmt (reasonml#2701) Drop the result dependency (reasonml#2703) Remove old CI and test.sh (reasonml#2705) Migrate tests to cram suite (reasonml#2694) Make sure win doesnt break when importing (reasonml#2700) Fix top level extensions (reasonml#2693) Install before importing deps Update package json and install esy normally Re-arrange esy install Reduce install esy time Use master branch instead of main Ignore _export from esy Rename refmt_test to test Add esy-ci and opam-ci Remove jbuild-ignore chore: update nix flake (reasonml#2692) chore(readme): clarify 3.9 is unreleased ...
SanderSpies
added a commit
to SanderSpies/reason
that referenced
this pull request
Apr 18, 2023
* master: (38 commits) chore: remove old BS_NO_COMPILER_PATCH flag (reasonml#2710) Improve printing of modules types with one line inside (reasonml#2709) generate opam files with dune (reasonml#2704) Fix version on refmt (reasonml#2701) Drop the result dependency (reasonml#2703) Remove old CI and test.sh (reasonml#2705) Migrate tests to cram suite (reasonml#2694) Make sure win doesnt break when importing (reasonml#2700) Fix top level extensions (reasonml#2693) Install before importing deps Update package json and install esy normally Re-arrange esy install Reduce install esy time Use master branch instead of main Ignore _export from esy Rename refmt_test to test Add esy-ci and opam-ci Remove jbuild-ignore chore: update nix flake (reasonml#2692) chore(readme): clarify 3.9 is unreleased ...
SanderSpies
added a commit
to SanderSpies/reason
that referenced
this pull request
May 4, 2023
* master: fix: binary parser (reasonml#2713) Improve functor printing. (reasonml#2683) chore: remove old BS_NO_COMPILER_PATCH flag (reasonml#2710) Improve printing of modules types with one line inside (reasonml#2709) generate opam files with dune (reasonml#2704) Fix version on refmt (reasonml#2701) Drop the result dependency (reasonml#2703) Remove old CI and test.sh (reasonml#2705) Migrate tests to cram suite (reasonml#2694) Make sure win doesnt break when importing (reasonml#2700) Fix top level extensions (reasonml#2693) Install before importing deps Update package json and install esy normally Re-arrange esy install Reduce install esy time Use master branch instead of main Ignore _export from esy Rename refmt_test to test Add esy-ci and opam-ci Remove jbuild-ignore
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While migrating to Reason 3.8.2 (and above) found a bug when printing extensions. It might be hidden a long time ago since most extensions aren't top-level.