cumbersome flag attributes

[dianaoigo]

@panglesd i have made some progress from the very helpful pointers you gave. kindly review and advise.
In my solution, i added specification in the src/attribute.mli then declared the attribute with a unit continuation. an empty payload pattern is created and passed to declare_with_all_args with a unit continuation. The has_flag function then checks if the attribute is present for the given value of x and returns a boolean value accordingly, with an optional mark_as_seen parameter that can be used to mark the attribute as seen if needed.

[marrious11]

Nice one @dianaoigo.
Could you try to add a DCO check

There is a requirement in ppxlib's to have signed commits... I think you just need to run:

git commit --amend --signoff

and force push to replace the current commit with a signed commit.

[panglesd]

Thanks!

The code is perfect. You need a few things to make the CI happy though:

• The code needs to be formatted. For this, you need the right version of ocamlformat installed (0.24.1), and to run dune fmt --auto-promote.
• As marrious11 said, you need to "sign-off" all your commits: they need to be created with the --sign-off argument.
• You need to add an entry in the Changes.md file!

And to make the documentation happy, you need to move your new entries in the signature. Indeed, the .mli file is used to generate the API page https://ocaml.org/p/ppxlib/latest/doc/Ppxlib/Attribute/index.html. Since you added the entries at the bottom of the file, they will be rendered at the bottom of the page. I think it would be more logic to have declare_flag next to the declare function, and has_flag next to get.
Finally, you should add "doc-comments" (the comments enclosed in (** ... *)) associated to the new values, that describe what they do.

[dianaoigo]

@panglesd thank you so much for your guidance. switching between working on different projects sometimes makes one forget some things. i will make the changes asap.

[Burnleydev1]

Hello @dianaoigo, I think to sign-off the commits that have already been pushed,

Use git log to find the hash of the commit that you want to edit.

Run git rebase -i <enter the commit hash here> to start a rebase session that includes the commit you want to edit and its parent commit. This will open a text editor with a list of commits and instructions for how to edit them.

Find the line that corresponds to the commit you want to edit, and change the command from pick to edit. Save and close the file.

Run git commit --amend -s to make changes to the commit. This will open a text editor where you can modify the commit message and/or the contents of the commit.

After making your changes, save and close the file.

Run git rebase --continue to continue the rebase process. This will apply your changes and modify the commit history.
When you're done, run git push --force-with-lease origin cumbersome_flag_attributes

[dianaoigo]

@panglesd after some serious fight and going in circles, i believe everything is in order.

[dianaoigo]

@Burnleydev1 @marrious11 thank you so much for the help

[panglesd]

@dianaoigo nice job! Indeed, all commits are now signed :)

I still have a few comments:

• As you can see in the list of modifications, your commit introduced many changes in the Changes.md file that are not necessary. Usually, it's because your editor reformatted the file at some point, which changed the layout. It would be better to have only the modification relevant to your change entry.
• The changelog (the Changes.md file) is used for users of ppxlib to know quickly the set of ppxlib new features/bugfixes that happened in between each release. So you should not add a 0.30.0 section, but add your entry to the unreleased section. Moreover, the text should be informative for someone who knows ppxlib, but who does not know about this PR or the corresponding issue. You should precise what it is about (e.g. attributes with no payload that can be checked for presence).
• Similarly, the doc comments should not just describe the type: that information is already there! The doc comment should explain what this function is for, what it does (only the comment in the .mli file is important, since it is the one rendered in the API.)

Writing good doc-comment is not an easy job, especially for newcomers in this codebase, but it is a very good exercise!

[dianaoigo]

@panglesd thank you for the guidance, i will familiarize myself more with writing good-doc. Do i still need to make the changes suggested?

[panglesd]

Unless you don't want to or don't have time (which I'll perfectly understand and do the changes myself!), yes the changes are necessary to get the PR merged!

[dianaoigo]

@panglesd i would love to do them but i will kindly request that you do them so that i can learn from what you do

[panglesd]

@dianaoigo I finally fixed the docstrings! (Sorry for the long delay).

I added a non raising version of has_flag: has_flag_res.
I also spotted a mistake in your handling of mark_as_seen. That variable should be passed to get_res. Your previous handling of it did not have a real effect!

Finally, I added a test.

Feel free to ask any question if you want some clarifications!

[dianaoigo]

@panglesd you made it look too easy with writing the docs. I'll get it with more practice
In my code mark_as_seen only determines whether to set the seen variable to true but doesn't carry out any modification. basically mark_as_seen is supposed to be effect the behavior of get_res? Error handling part was a good tip, i'll remember that part always

[panglesd]

Thanks @dianaoigo !
Sorry for the slow merging process. Your changes will be incorporated in the next ppxlib release!