Fix cli cookies handling #209
Merged
Conversation
This file contains hidden or 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
Before, cookies handed in through the cli were added to the `Ast_mapper` cookies reference right away when reading the cli arguments. But in the case of a binary AST input, that reference gets reset to only contain the cookies from the ppx context when parsing that context. Therefore, cli cookies were ignored for binary AST inputs. This commit fixes that by adding the cli cookies to the `Ast_mapper` cookies reference after parsing the ppx context. Signed-off-by: Sonja Heinze <sonjaleaheinze@gmail.com>
Since the `-cookie` argument can also be used together with the `-as-ppx` argument and when using `run_as_ppx_rewriter` as entry point, it should form part of shared_args instead of standalone_args. Otherwise, the documentation doesn't reflect the behavior. Signed-off-by: Sonja Heinze <sonjaleaheinze@gmail.com>
ghost
approved these changes
Jan 18, 2021
NathanReb
added a commit
to NathanReb/opam-repository
that referenced
this pull request
Jan 20, 2021
CHANGES: - Driver (important for bucklescript): handling binary AST's, accept any supported version as input; preserve that version (ocaml-ppx/ppxlib#205, @pitag-ha) - `-as-ppx`: take into account the `-loc-filename` argument (ocaml-ppx/ppxlib#197, @pitag-ha) - Add input name to expansion context (ocaml-ppx/ppxlib#202, @pitag-ha) - Add Driver.V2: give access to expansion context in whole file transformation callbacks of `register_transformation` (ocaml-ppx/ppxlib#202, @pitag-ha) - Driver: take `-cookie` argument into account, also when the input is a binary AST (@pitag-ha, ocaml-ppx/ppxlib#209) - `run_as_ppx_rewriter`: take into account the arguments `-loc-filename`, `apply` and `dont-apply` (ocaml-ppx/ppxlib#205, @pitag-ha) - Location.Error: add functions `raise` and `update_loc` (ocaml-ppx/ppxlib#205, @pitag-ha)
NathanReb
added a commit
to NathanReb/opam-repository
that referenced
this pull request
Jan 22, 2021
CHANGES: - Fix ppxlib.traverse declaration and make it a deriver and not a rewriter (ocaml-ppx/ppxlib#213, @NathanReb) - Driver (important for bucklescript): handling binary AST's, accept any supported version as input; preserve that version (ocaml-ppx/ppxlib#205, @pitag-ha) - `-as-ppx`: take into account the `-loc-filename` argument (ocaml-ppx/ppxlib#197, @pitag-ha) - Add input name to expansion context (ocaml-ppx/ppxlib#202, @pitag-ha) - Add Driver.V2: give access to expansion context in whole file transformation callbacks of `register_transformation` (ocaml-ppx/ppxlib#202, @pitag-ha) - Driver: take `-cookie` argument into account, also when the input is a binary AST (@pitag-ha, ocaml-ppx/ppxlib#209) - `run_as_ppx_rewriter`: take into account the arguments `-loc-filename`, `apply` and `dont-apply` (ocaml-ppx/ppxlib#205, @pitag-ha) - Location.Error: add functions `raise` and `update_loc` (ocaml-ppx/ppxlib#205, @pitag-ha)
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.
Before, cookies handed in through the cli were added to the
Ast_mappercookies reference right away when reading the cli arguments. But in the case of a binary AST input, that reference gets reset to only contain the cookies from the ppx context when parsing that context (here the reference gets reset instead of appending to it). Therefore, cli cookies were ignored for binary AST inputs.This commit fixes that by adding the cli cookies to the
Ast_mappercookies reference after parsing the ppx context.Probably related to janestreet/ppx_expect#26 and to the last comment in janestreet/ppx_inline_test#1