Add "ns" and "res" as reserved namespaces #388
Merged
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
Signed-off-by: David Sancho Moreno <dsnxmoreno@gmail.com>
davesnx
requested review from
ceastlund,
panglesd and
pitag-ha
as code owners
|
I take it the |
ceastlund
approved these changes
Feb 22, 2023
|
Good question, I added by inertia and needed to check what it was. It seems like an old name from the syntax called "napskinscript". It's used in a few places from the rescript-lang/syntax: https://github.com/search?q=repo%3Arescript-lang%2Fsyntax%20%22ns.&type=code Maybe worth adding a note to all reserved names? |
|
Hi @davesnx, thanks! Could you add a changelog entry? Apart from that, LGTM as well! I'm also thinking that it might be useful to add a comment in the code pointing out that these two namespaces are for rescript compatibility. As @ceastlund has pointed out above, that's not immediately obvious from their name. As a side-note:
Is that the idea? I remember explicitly adding bucklescript support some time ago: #205 (I know lot's of things have changed in the bucklescript/rescript world since back then, but am not very aware of the details) |
davesnx
added a commit
to davesnx/ppxlib
that referenced
this pull request
Feb 23, 2023
Signed-off-by: David Sancho Moreno <dsnxmoreno@gmail.com>
Signed-off-by: David Sancho Moreno <dsnxmoreno@gmail.com>
davesnx
force-pushed
the
Add-res-attribute-as-ignored
branch
from
581482d
to
4c2791d
Compare
|
Hi @pitag-ha
BuckleScript support with the driver and those changes enable to work well with ReScript. Their core team is against ppxes and they discourage to use them but there's a few popular ones that are widely used. If ever the direction of ReScript is to break completly from ppxlib we can always remove those pieces (I'm happy to be that person!) |
pitag-ha
pushed a commit
that referenced
this pull request
Mar 14, 2023
Signed-off-by: David Sancho Moreno <dsnxmoreno@gmail.com>
|
Thanks! |
tmattio
added a commit
to tmattio/opam-repository
that referenced
this pull request
Jun 20, 2023
CHANGES: - Adopt the OCaml Code of Conduct on the repo (ocaml-ppx/ppxlib#426, @pitag-ha) - Clean up misleading attribute hints when declared for proper context. (ocaml-ppx/ppxlib#425, @ceastlund) - Ast_pattern now has ebool, pbool helper, and a new map.(ocaml-ppx/ppxlib#402, @Burnleydev1) - multiple errors are now reported in `metaquot`. (ocaml-ppx/ppxlib#397, @Burnleydev1) - Add `Attribute.declare_with_attr_loc` (ocaml-ppx/ppxlib#396, @dvulakh) - Add "ns" and "res" as reserved namespaces(ocaml-ppx/ppxlib#388, @davesnx) - Make quoter `let` binding non-recursive (ocaml-ppx/ppxlib#401, @sim642) - Fix failure of 'lift_map_with_context' in traverse by compile-time evaluation of 'fst' and 'snd' (ocaml-ppx/ppxlib#390, @smuenzel) - Driver: Bias the mapping from magic to version towards the current version, as it is usually the common case and it helps when magic numbers are ambiguous (such as on development versions) (ocaml-ppx/ppxlib#409, @shym) - Remove unnecessary test dependencies towards base and stdio (ocaml-ppx/ppxlib#421, @kit-ty-kate) - Update description to reflect that `ppxlib` contains more than a library (ocaml-ppx/ppxlib#422, @pitag-ha) - Add support for OCaml 5.1, excluding OCaml `5.1.0~alpha1` (ocaml-ppx/ppxlib#428, @shym, @Octachron , @pitag-ha, @panglesd) - Driver: Fix `-locations-check` option for coercions with ground (ocaml-ppx/ppxlib#428, @Octachron)
tmattio
added a commit
to tmattio/opam-repository
that referenced
this pull request
Jun 20, 2023
CHANGES: - Adopt the OCaml Code of Conduct on the repo (ocaml-ppx/ppxlib#426, @pitag-ha) - Clean up misleading attribute hints when declared for proper context. (ocaml-ppx/ppxlib#425, @ceastlund) - Ast_pattern now has ebool, pbool helper, and a new map.(ocaml-ppx/ppxlib#402, @Burnleydev1) - multiple errors are now reported in `metaquot`. (ocaml-ppx/ppxlib#397, @Burnleydev1) - Add `Attribute.declare_with_attr_loc` (ocaml-ppx/ppxlib#396, @dvulakh) - Add "ns" and "res" as reserved namespaces(ocaml-ppx/ppxlib#388, @davesnx) - Make quoter `let` binding non-recursive (ocaml-ppx/ppxlib#401, @sim642) - Fix failure of 'lift_map_with_context' in traverse by compile-time evaluation of 'fst' and 'snd' (ocaml-ppx/ppxlib#390, @smuenzel) - Driver: Bias the mapping from magic to version towards the current version, as it is usually the common case and it helps when magic numbers are ambiguous (such as on development versions) (ocaml-ppx/ppxlib#409, @shym) - Remove unnecessary test dependencies towards base and stdio (ocaml-ppx/ppxlib#421, @kit-ty-kate) - Update description to reflect that `ppxlib` contains more than a library (ocaml-ppx/ppxlib#422, @pitag-ha) - Add support for OCaml 5.1, excluding OCaml `5.1.0~alpha1` (ocaml-ppx/ppxlib#428, @shym, @Octachron , @pitag-ha, @panglesd) - Driver: Fix `-locations-check` option for coercions with ground (ocaml-ppx/ppxlib#428, @Octachron)
tmattio
added a commit
to tmattio/opam-repository
that referenced
this pull request
Jun 20, 2023
CHANGES: - Adopt the OCaml Code of Conduct on the repo (ocaml-ppx/ppxlib#426, @pitag-ha) - Clean up misleading attribute hints when declared for proper context. (ocaml-ppx/ppxlib#425, @ceastlund) - Ast_pattern now has ebool, pbool helper, and a new map.(ocaml-ppx/ppxlib#402, @Burnleydev1) - multiple errors are now reported in `metaquot`. (ocaml-ppx/ppxlib#397, @Burnleydev1) - Add `Attribute.declare_with_attr_loc` (ocaml-ppx/ppxlib#396, @dvulakh) - Add "ns" and "res" as reserved namespaces(ocaml-ppx/ppxlib#388, @davesnx) - Make quoter `let` binding non-recursive (ocaml-ppx/ppxlib#401, @sim642) - Fix failure of 'lift_map_with_context' in traverse by compile-time evaluation of 'fst' and 'snd' (ocaml-ppx/ppxlib#390, @smuenzel) - Driver: Bias the mapping from magic to version towards the current version, as it is usually the common case and it helps when magic numbers are ambiguous (such as on development versions) (ocaml-ppx/ppxlib#409, @shym) - Remove unnecessary test dependencies towards base and stdio (ocaml-ppx/ppxlib#421, @kit-ty-kate) - Update description to reflect that `ppxlib` contains more than a library (ocaml-ppx/ppxlib#422, @pitag-ha) - Add support for OCaml 5.1, excluding OCaml `5.1.0~alpha1` (ocaml-ppx/ppxlib#428, @shym, @Octachron , @pitag-ha, @panglesd) - Driver: Fix `-locations-check` option for coercions with ground (ocaml-ppx/ppxlib#428, @Octachron)
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.
Hi,
This PR adds 2 reserved namespaces "ns" and "res" which enables the possibility to use ppxlib with ReScript.
Further info: zth/rescript-relay#372 and davesnx/styled-ppx#308
A little bit of context
Using ppxlib to write a ppx for ReScript is quite common (not so much for the parsetree versioning, but rather the ppxlib APIs) but I believe it could conflict with the idea of ppxlib to just be for OCaml/Reason development.
I might agree, but the change seems rather small and harmless. ppxlib can always remove the namespace in a new release and make ReScript team/community fork ppxlib or solve it their own way.
I would rather have ppxlib with this change and enjoy all new work than rather keep a fork with these 2 lines.
Commit references from zth/ppxlib
zth@32f8339 and zth@b5b48e0