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

Having flag attributes is cumbersome with current API #404

Closed
panglesd opened this issue Mar 23, 2023 · 13 comments
Closed

Having flag attributes is cumbersome with current API #404

panglesd opened this issue Mar 23, 2023 · 13 comments

Comments

@panglesd
Copy link
Collaborator

With current API, one needs to create an "empty payload" pattern:

let p = Ast_pattern.(pstr nil)

then register the attribute with a unit continuation:

let attr = Attribute.declare name context p ()

And finally, when getting it, turn the unit option type into a bool type:

match Attribute.get attr x with
    | Some () -> true
    | None -> false

All of this could be simplified with the following API addition:

val declare_flag : string -> 'a Context.t -> ('a, unit) t
val has_flag : ('a, unit) t -> ?mark_as_seen:bool -> 'a -> bool

(reported by @sim642 in ocaml-ppx/ppx_deriving#263 (comment))

@dianaoigo
Copy link
Contributor

@panglesd i would like to work on this issue

@panglesd
Copy link
Collaborator Author

Yes, sure!

Do not hesitate to ask for clarification if something in the issue is obscure!

@dianaoigo
Copy link
Contributor

@panglesd i have forked and cloned the project . looking at the project, would it be a good idea to make the suggested changes in the src/ast_pattern.ml file? I am still learning and understanding more about the ocaml ecosystem.

@panglesd
Copy link
Collaborator Author

Attributes are part of the OCaml syntax, as in:

type t = {
  a : int [@foo "an attribute named foo whose payload is a string"]
}

Ast_pattern is a module to build "pattern-matches of the AST". It is used in several places in ppxlib, mostly to extract payloads in extension nodes and attributes. The documentation on Ast_pattern is available here.

ppxlib allows "specifying" attributes and their payloads, from a name, a context and an ast pattern. This is the declare function. From a declared attribute, one can then extract the value in the payload, using the ast-pattern: this is get.

Here, the issue is about adding support for a common case, where the attributes have no payload, the only thing we are interested in is whether they are present or not. For this, I propose to add the declare_flag and has_flag function. According to you, in which module in the API should they belong?

@dianaoigo
Copy link
Contributor

@panglesd i don't know why i haven't been able to see till i restarted my machine today. At some point i thought you were ignoring me. I was actually caught between the ast_pattern and attrribute model i just wasn't sure which. for now i believe attribute module would be the best way since it's where declaration and retrieval of attributes is done.

@dianaoigo
Copy link
Contributor

@panglesd since i restarted my machine i am having serious problems. now when i run _dune build _ i get the error
File "dune-project", line 6, characters 1-5:
6 | (cram enable)
^^^^
Error: Unknown field cram
and when i try reinstalling the dependencies using _opam install --deps-only --with-test .
_ i get the error
[ERROR] Package conflict!

  • Missing dependency:
    • ppxlib < 0.1.0
      no matching version

No solution found, exiting
i am using ubuntu

@dianaoigo
Copy link
Contributor

dianaoigo commented Mar 28, 2023

@pitag-ha i am having some problems. i restarted my machine after almost a week and it must have corrupted everything. now i can't even build or test as shown in the comment above

@panglesd
Copy link
Collaborator Author

Hello @dianaoigo !
First, if you ever feel that I am ignoring you, please ping me. It could happen that I miss one gh notification.
This was not the case here: I thought you were busy working on this or some other project since you did not reply to my clarifications.

The error message when running dune build seems to indicate that dune has a too low version. Reinstalling the dependencies was a good move, but the error you get is strange (I think I already had it once, I think it is due to ppxlib-bench.opam).

You could try:

Let me know if that unblocks the situation!

@panglesd
Copy link
Collaborator Author

Also, it is strange that this happened after a machine reboot... Maybe you changed the opam switch, or your environment is not up to date with the switch you have chosen? (eval $(opam env))

@dianaoigo
Copy link
Contributor

dianaoigo commented Mar 29, 2023

I uninstalled everything dune, opam ocaml, reinstalled and updated. somehow that solved the issue. also learnt that any dune version below 2.7 will throw some crazy errors. I have been working on a different ocaml project maybe that was the cause. Could you offer a little more insight on how to proceed?

@panglesd
Copy link
Collaborator Author

panglesd commented Mar 30, 2023

@dianaoigo I already gave all the generic information I could in #404 (comment) and #404 (comment), in order to help you more I would need a more detailed question. For instance, I need to know what is clear and what is unclear to you, to answer the right thing. There is a nice blog post about asking good questions here that explains that very well!

@sim642
Copy link
Contributor

sim642 commented Mar 7, 2024

This should be closed by #408 I think.

@panglesd
Copy link
Collaborator Author

panglesd commented Mar 7, 2024

Indeed, thanks!

@panglesd panglesd closed this as completed Mar 7, 2024
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