Skip to content
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

Give semantics to reserving attribute namespace with dots #443

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

ncik-roberts
Copy link
Contributor

Resolves #442.

This PR gives different semantics to reserving attribute namespaces that contain dots, like "foo.bar". The previous semantics: reserving this namespace has no effect. The new semantics: reserving this namespaces causes attributes of the form [@foo.bar] or [@foo.bar.*] to be treated as reserved.

This is technically a backward-incompatible change, as existing reservations of "foo.bar" will start to have an effect. But it's hard to imagine anyone actually intending these reservations to have no effect, and I claim the new semantics is a more reasonable way to handle this case.

@panglesd
Copy link
Collaborator

Thanks a lot for the PR! I haven't looked at the implementation, but from the description, the change seems an improvement!

Apologies for the very long delay in reviewing. I have very few time to allocate to ppxlib, but I hope to be able to review this week.

Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Both semantics and implementation look good to me. Also thanks for the code comments, they've made review a lot fatster!

@pitag-ha
Copy link
Member

PD: I've just added a changelog entry:

Attribute namespaces: Fix semantics of reserving multi-component namespaces (#443, @ncik-roberts)

Let me know if you'd prefer something else.

@ncik-roberts
Copy link
Contributor Author

Thanks for the review. That changelog entry looks good to me.

@pitag-ha pitag-ha merged commit 7cc8cfd into ocaml-ppx:main Sep 18, 2023
4 checks passed
avsm pushed a commit to ocaml/opam-repository that referenced this pull request Oct 5, 2023
CHANGES:

- Fix support for OCaml 5.1: migrated code preserves generative
  functor warnings, without creating more. Locations are better
  preserved. (ocaml-ppx/ppxlib#432, @pitag-ha, @panglesd)

- Driver: Add `-unused-code-warnings` command-line flag. (ocaml-ppx/ppxlib#444, @ceastlund)

- Add `?warning` flag to `Deriving.Generator.make`. (ocaml-ppx/ppxlib#440, @jacksonzou123 via @ceastlund)

- Restore the "path_arg" functionality in the V3 API (ocaml-ppx/ppxlib#431, @ELLIOTTCABLE)

- Expose migration/copying/etc. functions for all AST types needed by `Pprintast` (ocaml-ppx/ppxlib#454, @antalsz)

- Preserve quoted attributes on antiquotes in metaquot (ocaml-ppx/ppxlib#441, @ncik-roberts)

- Attribute namespaces: Fix semantics of reserving multi-component namespaces (ocaml-ppx/ppxlib#443, @ncik-roberts)
@ncik-roberts ncik-roberts deleted the reserve-sub-namespaces branch October 26, 2023 18:52
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- Fix support for OCaml 5.1: migrated code preserves generative
  functor warnings, without creating more. Locations are better
  preserved. (ocaml-ppx/ppxlib#432, @pitag-ha, @panglesd)

- Driver: Add `-unused-code-warnings` command-line flag. (ocaml-ppx/ppxlib#444, @ceastlund)

- Add `?warning` flag to `Deriving.Generator.make`. (ocaml-ppx/ppxlib#440, @jacksonzou123 via @ceastlund)

- Restore the "path_arg" functionality in the V3 API (ocaml-ppx/ppxlib#431, @ELLIOTTCABLE)

- Expose migration/copying/etc. functions for all AST types needed by `Pprintast` (ocaml-ppx/ppxlib#454, @antalsz)

- Preserve quoted attributes on antiquotes in metaquot (ocaml-ppx/ppxlib#441, @ncik-roberts)

- Attribute namespaces: Fix semantics of reserving multi-component namespaces (ocaml-ppx/ppxlib#443, @ncik-roberts)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reserving an attribute namespace with a dot has unexpected semantics
3 participants