-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fix issue 503 #504
Fix issue 503 #504
Conversation
Signed-off-by: Javier Chavarri <[email protected]>
4661a93
to
3db898d
Compare
Signed-off-by: Javier Chavarri <[email protected]>
Signed-off-by: Javier Chavarri <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to report and fix this bug!
If I understand it correctly you should apply the same change to the Pexp_newtype
node below as well!
The tests for the 501<->502 migrations are pretty unreadable in their current state unfortunately and I'd rather avoid adding more of those. I suggest you instead take an approach closer to how we test the ocaml.ppx.context
migration (see the driver.ml
, dune
and run.t
files). The idea is that you'd move both your tests to their own folder and write a custom no-op (as in, no actual transformaion) driver that run specific migrations and could here, display the location of specific bits of the AST, such as the Pexp_fun
with the bar
argument on the standard output.
I'm happy to help with the refactoring of those tests if needed so please let me know!
Signed-off-by: Javier Chavarri <[email protected]>
Signed-off-by: Javier Chavarri <[email protected]>
Signed-off-by: Javier Chavarri <[email protected]>
Signed-off-by: Javier Chavarri <[email protected]>
Hi @NathanReb thanks for the quick review :) I applied the following changes:
I also removed the previous location tests. Please lmk if something like this is what you had in mind. |
Signed-off-by: Javier Chavarri <[email protected]>
Signed-off-by: Javier Chavarri <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly what I had in mind, thanks for taking the time to refactor it and add the changelog entry!
- Fix a bug where `Code_path.main_module_name` would not properly remove extensions from the filename and therefore return an invalid module name. (ocaml-ppx/ppxlib#512, @NathanReb) - Add `-unused-type-warnings` flag to the driver to allow users to disable only the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#511, @mbarbin, @NathanReb) - Make `-unused-code-warnings` flag to the driver also controls the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#510, @mbarbin, @NathanReb) - Driver: Add `-unused-code-warnings=force` command-line flag argument. (ocaml-ppx/ppxlib#490, @mbarbin) - new functions `Ast_builder.{e,p}list_tail` that take an extra tail expression/pattern argument parameter compared to `Ast_builder.{e,p}list`, so they can build ASTs like `a :: b :: c` instead of only `[ a; b ]`. (ocaml-ppx/ppxlib#498, ocaml-ppx/ppxlib#502, @v-gb, @NathanReb) - Fix `Longident.parse` so it also handles indexing operators such as `.!()`, `.%(;..)<-`, or `Vec.(.%())` (ocaml-ppx/ppxlib#494, @Octachron) - Add a `special_function'` variant which directly takes a `Longident.t` argument to avoid the issue that `Longident.t` cover distinct syntaxic classes which cannot be easily parsed by a common parser (ocaml-ppx/ppxlib#496, @Octachron). - Keep location ranges consistent when migrating `Pexp_function` nodes from 5.2+ to older versions (ocaml-ppx/ppxlib#504, @jchavarri) - Fix `-locations-check` behaviour so it is no longer required to pass `-check` as well to enable location checks. (ocaml-ppx/ppxlib#506, @NathanReb) Signed-off-by: Nathan Rebours <[email protected]>
- Fix a bug where `Code_path.main_module_name` would not properly remove extensions from the filename and therefore return an invalid module name. (ocaml-ppx/ppxlib#512, @NathanReb) - Add `-unused-type-warnings` flag to the driver to allow users to disable only the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#511, @mbarbin, @NathanReb) - Make `-unused-code-warnings` flag to the driver also controls the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#510, @mbarbin, @NathanReb) - Driver: Add `-unused-code-warnings=force` command-line flag argument. (ocaml-ppx/ppxlib#490, @mbarbin) - new functions `Ast_builder.{e,p}list_tail` that take an extra tail expression/pattern argument parameter compared to `Ast_builder.{e,p}list`, so they can build ASTs like `a :: b :: c` instead of only `[ a; b ]`. (ocaml-ppx/ppxlib#498, ocaml-ppx/ppxlib#502, @v-gb, @NathanReb) - Fix `Longident.parse` so it also handles indexing operators such as `.!()`, `.%(;..)<-`, or `Vec.(.%())` (ocaml-ppx/ppxlib#494, @Octachron) - Add a `special_function'` variant which directly takes a `Longident.t` argument to avoid the issue that `Longident.t` cover distinct syntaxic classes which cannot be easily parsed by a common parser (ocaml-ppx/ppxlib#496, @Octachron). - Keep location ranges consistent when migrating `Pexp_function` nodes from 5.2+ to older versions (ocaml-ppx/ppxlib#504, @jchavarri) - Fix `-locations-check` behaviour so it is no longer required to pass `-check` as well to enable location checks. (ocaml-ppx/ppxlib#506, @NathanReb) Signed-off-by: Nathan Rebours <[email protected]>
Fixes #503.
In the first commit, notice the difference in the ending location of the
Pexp_fun
with the labelledbar
.In 5.1:
ppxlib/test/501_migrations/one_migration.t
Line 1726 in 4661a93
In 5.2:
ppxlib/test/502_migrations/one_migration.t
Line 719 in 4661a93
The second commit updates
Migrate_502_501
so that the ranges of locations ofPexp_fun
stay consistent with the behavior in 5.1.