-
Notifications
You must be signed in to change notification settings - Fork 102
Add Mod.apply_unit #611
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
Open
craff
wants to merge
2
commits into
ocaml-ppx:main
Choose a base branch
from
craff:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Add Mod.apply_unit #611
+5
−1
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
Signed-off-by: lexa <[email protected]> Signed-off-by: craff <[email protected]>
Signed-off-by: christophe Raffalli <[email protected]> Signed-off-by: craff <[email protected]>
Collaborator
|
Hi @craff, thanks for this adding this. Is there any reason you are not using the functions supplied by |
Author
|
Patrick Ferris ***@***.***> writes:
*
patricoferris left a comment (ocaml-ppx/ppxlib#611)
Hi @craff, thanks for this adding this.
Is there any reason you are not using the functions supplied by Ast_builder, in this case I think it would
bepmod_apply_unit?
Hello,
It is what I use to define Mod.apply_unit. I think le ast_helper_lite is
very nice as it evolves less than the parsetree itself.
Say this, I think an alternative would be to have Mod.apply checking if
the structure is empty ?
If you think it is better I could modify my PR.
Cheers,
Christophe
… —
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
--
Christophe Raffalli
web: https://raffalli.eu
|
Collaborator
Author
|
Patrick Ferris ***@***.***> writes:
*
patricoferris left a comment (ocaml-ppx/ppxlib#611)
Thanks for the response @craff -- the reason I ask is we have just announced a roadmap that includes the (slow) deprecation for
Ast_helper and it would be great to hear your thoughts on that.
I think Ast_helper is nice, because it offers a way to make your code
easier to maintain through default value for parameter or keeping old
constructor.
If there way a similar approach to destructors it would be nice.
As far as I am concern I tend to do as much as I can using metaquot and
ast_helper and almost never need modification of my code. If Mod.apply
where modified to call Pmod_apply_unit when neede, I would not have has
to modify pacomb.
On the other hand if metaquot were complete (I mean a syntax entry point
for every type in the ast both for expressions and pattern). Then I think
I would only use that, and this would be the ultimate solution. Though I have
not look recently if metaquot was extended.
Cheers,
Christophe
… —
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
--
Christophe Raffalli
web: https://raffalli.eu
|
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.
Solves #610. Just modified ast_helperlite.mli? And tested on my code.