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

More documentation #180

Merged
merged 2 commits into from
Jan 17, 2021
Merged

More documentation #180

merged 2 commits into from
Jan 17, 2021

Conversation

v-gb
Copy link
Contributor

@v-gb v-gb commented Dec 21, 2020

A lot of the api in core.mli has barely any documentation, and the behavior is not obvious at all. Or sometimes, the behavior is the expected one, but since it's not documented, you end up having to test it to make sure. So this is trying to address that.

The first commit adds Re.Group.get_opt because it sounds like a straightforward omission, and I wanted it for an example in the documentation. The second commit is pure documentation. I think the documentation I wrote is correct, and I tested some of it, but I am not 100% sure.

I wanted to add something about marks, but I didn't, because I don't see the point of using them over using an empty group (performance maybe? or is just simpler api? or do they simply because they are used internally so it's easy to provide?).

@rgrinberg rgrinberg requested a review from Drup December 24, 2020 22:44
@v-gb
Copy link
Contributor Author

v-gb commented Jan 16, 2021

Ping? This addresses, at least partially, multiple issues (#178, #177, #161, #150, #128).

@rgrinberg
Copy link
Member

@Drup might reading over this one?

Copy link
Collaborator

@Drup Drup left a comment

Choose a reason for hiding this comment

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

I started reviewing, but got caught up with some other work, sorry.

The conclusion is: I dislike many of the phrasing: it's negative/self-bashing on multiple occasions which has no place in documentation; and uses various non-intuitive formulations; etc. However, pointing them out is going to be longer than just rephrasing things myself, and in any case, it's much better than nothing, so let's merge.

@Drup Drup merged commit 09c2745 into ocaml:master Jan 17, 2021
@v-gb
Copy link
Contributor Author

v-gb commented Jan 17, 2021

Thanks.

I'll change witness, because I'll admit that the phrasing for this one is aggressive (but I do think that given that it's unlikely to be fixed anytime soon, because it looks hard to fix, it's preferable to point out the issue in the doc).

But otherwise, while I'm sure some things can be formulated better, I don't know what's negative or self bashing. For instance, saying that exec str ~pos ~len is similar but not equivalent to exec (String.sub str ~pos ~len) is not a criticism, it's just a property that one naturally expects so I think it makes sense to point out that it doesn't necessarily hold and what makes it not hold. Like the documentation of List.map stating that it's not tail recursive.

@kit-ty-kate
Copy link
Member

Is there any release planned soon? Group.get_opt would be really nice to have

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.

4 participants