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

Remove Context_free.Rule.attr* #14

Open
ghost opened this issue Apr 12, 2018 · 7 comments
Open

Remove Context_free.Rule.attr* #14

ghost opened this issue Apr 12, 2018 · 7 comments

Comments

@ghost
Copy link

ghost commented Apr 12, 2018

These are only used for [@@deriving ...] and it's unlikely they'll ever be used for something else. Now that [@@deriving] is part of ppxlib, I think removing them and specializing the implementation to only support [@@deriving ...] would make the code simpler.

@NathanReb
Copy link
Collaborator

I'd like to give this a try in order to make my life easier with #61.

I'm trying to understand what the scope of this actually is.

My understanding so far is that we want to get rid of the attr_* smart constructor and replace them with something more in the line of Context_free.Rule.extension that would only allow you to declare a rule for the deriving attribute.
Removing these smart constructors is gonna break the API but I assume we don't consider this as a problem since they're probably only used in Deriving.

These smart constructors appear to be called a single time, on all the registered generators at once. I'm not sure exactly what the new API should look like.

I'm also guessing this is the occasion to leak a bit less about the internal types in Context_free.Rule.

@ghost
Copy link
Author

ghost commented Feb 26, 2019

My understanding so far is that we want to get rid of the attr_* smart constructor and replace them with something more in the line of Context_free.Rule.extension that would only allow you to declare a rule for the deriving attribute.

I was thinking to completely remove them and make Context_free depend on Deriving. i.e. context_free.ml would directly call functions from Deriving.

Removing these smart constructors is gonna break the API but I assume we don't consider this as a problem since they're probably only used in Deriving.

Agreed.

BTW, I remember that I tried this last year. My attempt is in the branch simplify-context-free. I don't remember exactly what's the status of this branch is, but it might be worth having a look. I couldn't easily get rid of the circular dependency between Context_free, Deriving and Driver, which was annoying and required forward references, but I don't know if we can do better though.

@kwshi
Copy link

kwshi commented Jul 30, 2019

Hmm-- I'm working on a project that requires [@@deriving]-like features, except on module declarations. As far as I can tell, [@@deriving] only currently supports types. If there is no plan to develop the attr-libraries and use only [@@deriving], what shall I do?

@ghost
Copy link
Author

ghost commented Aug 1, 2019

I know ppx_deriving allows this. It seems fine to add the same support in ppxlib.

@kwshi
Copy link

kwshi commented Aug 6, 2019

Does it, really? The API docs seem to indicate only support for type_ext and type_decl transformers (no mention of modules whatsoever), and every single one of the examples in the README only shows @@deriving being used on types, not modules.

@gasche
Copy link

gasche commented Aug 6, 2019

This message helped me realize that the API docs for ppx_deriving were way out of date, I just updated them. The new docs, and it does mention the hooks to operate on module types (not modules), which were implemented in ocaml-ppx/ppx_deriving#94.

@kwshi
Copy link

kwshi commented Aug 7, 2019

Thanks for updating the docs! My project, however, requires operating on a module expression, which so it seems is not provided by deriving.

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

No branches or pull requests

3 participants