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

Consistently use @raise tags in Stdlib docs #8644

Merged
merged 18 commits into from Jun 30, 2020
Merged

Consistently use @raise tags in Stdlib docs #8644

merged 18 commits into from Jun 30, 2020

Conversation

Et7f3
Copy link
Contributor

@Et7f3 Et7f3 commented Apr 24, 2019

All comment have the same shape
(** comment *)
Use of @raise tags when it was in text.
Do you think it worth ask for ocamldoc autodetect exception and try to detect which value cause error (by scanning code like if match ... with structure ?)

@dra27
Copy link
Member

dra27 commented Apr 25, 2019

I like the conversion to @raise, thanks (would you be happy to do similar in stdlib/list.mli?

I'm less thrilled about the necessity of reformatting double-spaces and exactly where *) marks appear.

Et7f3 added a commit to Et7f3/ocaml that referenced this pull request Apr 25, 2019
Use of @raise tags when it was in text. for list.mli file.
Like asked by @dra27 in ocaml#8644
@Et7f3
Copy link
Contributor Author

Et7f3 commented Apr 25, 2019

I have not modified double space in list.mli and change where *) mark appear.

@Et7f3
Copy link
Contributor Author

Et7f3 commented Apr 25, 2019

some function like combine don't have the value of Invalid_argument.
I may need to fix it 🤔 .
image
I will use grep for finding other examples in all the code.

@gasche
Copy link
Member

gasche commented Apr 26, 2019

I may need to fix it.

Please don't; the string payload of Invalid_argument occasionally changes, and it's a bad idea for users to rely on it.

@dra27
Copy link
Member

dra27 commented May 1, 2019

Please do try to test these - make world builds documentation by default and you can always do make -C ocamldoc html_doc to test changes.

@dra27
Copy link
Member

dra27 commented May 1, 2019

I leave it to @gasche to rule whether the whitespace alterations in listLabels.mli should be reverted before this is squashed, but it's otherwise good to go.

It's entirely up to you, @Et7f3, but a quick grep reveals arrayLabels.mli, array.mli, bigarray.mli, buffer.mli, bytesLabels.mli, bytes.mli, camlinternalFormat.ml, char.mli, digest.mli, filename.mli, float.mli, gc.mli, int32.mli, int64.mli, nativeint.mli, queue.mli, scanf.mli, stack.mli, stdlib.mli, stringLabels.mli, string.mli, sys.mli and weak.mli also contain "Raise", if you fancied changing some more.

A further improvement which could be applied here and more widely would be to remove the arguments in documentation comments from Invalid_argument and Failure. Arguably that should have been part of the change to the [@ocaml.warn_on_literal_pattern] attribute

@Et7f3
Copy link
Contributor Author

Et7f3 commented May 8, 2019

The modules CamlinternalFormat have a manual page and comment but comment is not displayed. Is it intended ? Or we should transform comment to ocamldoc format ?

How should I treat Raise {!Scanf.Scan_failure} @raise don't support it.

@gasche
Copy link
Member

gasche commented Sep 10, 2019

Unfortunately, I don't have the time right now to review this properly. @dra27, if you do have the time at some point, I would be happy to defer all decisions to you.

dra27 pushed a commit to Et7f3/ocaml that referenced this pull request Sep 27, 2019
Use of @raise tags when it was in text. for list.mli file.
Like asked by @dra27 in ocaml#8644
@damiendoligez
Copy link
Member

@dra27 the whitespace changes shouldn't prevent us from merging this.

@Et7f3:

  • CamlinternalFormat has a comment at the top that says (* No comments, OCaml stdlib internal use only. *). Please do not convert the comments.
  • Raise {!Scanf.Scan_failure}: are there many cases of this? If not, I would just drop the prefix. If there are many, you should keep them as-is until we add the feature to ocamldoc.
  • You'll need to rebase this PR as it has conflicts with the current trunk.

Et7f3 added a commit to Et7f3/ocaml that referenced this pull request Jan 30, 2020
Use of @raise tags when it was in text. for list.mli file.
Like asked by @dra27 in ocaml#8644
Et7f3 added a commit to Et7f3/ocaml that referenced this pull request Jan 30, 2020
Use of @raise tags when it was in text. for list.mli file.
Like asked by @dra27 in ocaml#8644
Et7f3 added a commit to Et7f3/ocaml that referenced this pull request Jan 30, 2020
Use of @raise tags when it was in text. for list.mli file.
Like asked by @dra27 in ocaml#8644
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

There are four instances of Failure and Invalid_argument still with an argument (I don't think ocamldoc renders @raise Exception ["argument"] very nicely, though I haven't actually checked).

I'm happy for this to go in without a Changes entry, unless you want to push one (potentially with those four adjustments) and then I'll (finally) merge this, thanks!

stdlib/list.mli Outdated Show resolved Hide resolved
stdlib/list.mli Outdated Show resolved Hide resolved
stdlib/list.mli Outdated Show resolved Hide resolved
stdlib/list.mli Outdated Show resolved Hide resolved
Et7f3 and others added 10 commits June 30, 2020 09:44
All comment have the same shape
`(**
   comment
 *)`
Use of @raise tags when it was in text.
Do you think it worth ask for ocamldoc autodetect exception and try to detect which value cause error (by scanning code like if match ... with structure ?)
Use of @raise tags when it was in text. for list.mli file.
Like asked by @dra27 in ocaml#8644
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@Et7f3
Copy link
Contributor Author

Et7f3 commented Jun 30, 2020

My sed.js used:

const fs = require("fs")

const my_args = process.argv.slice(2)
my_args.forEach(name => {
  const code = fs.readFileSync(name)
  fs.writeFileSync(name, code.toString().replace(/\s*Raise \[(Not_found|Invalid_argument|Division_by_zero|End_of_file|Sys_error|Failure)\]/gm, "\n   @raise $1"))
})

I launched with: node sed.js $(find stdlib -name "*.mli")

(I haven't used ocaml stdlib because I think the nodejs has more utility function and I feel dumb to have beginned this task by hand 🤡)
EDIT:
I have used 3 spaces because it was the number of space used in the fist file edited.

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Nice! Two small nits remain. The use of 3 spaces before @raise means that the source-formatting is off in many places. It won't affect the HTML, but if you're doing it all with a script why not:

  • Preprocess by hand the small number of places where Raise does not begin on a newline
  • Capture the \s in the regexp and use the capture group for the spacing instead of a hard-coded 3 spaces?

stdlib/listLabels.mli Outdated Show resolved Hide resolved
stdlib/scanf.mli Outdated Show resolved Hide resolved
Et7f3 and others added 2 commits June 30, 2020 11:09
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@Et7f3
Copy link
Contributor Author

Et7f3 commented Jun 30, 2020

Does I modify failwith description or it is obvious ?

@Et7f3
Copy link
Contributor Author

Et7f3 commented Jun 30, 2020

The use of 3 spaces before @raise means that the source-formatting is off in many places.

In many files 2/3 or 3/4 spaces are used.

Capture the \s in the regexp and use the capture group for the spacing instead of a hard-coded 3 spaces?

sometime we have text Raise so \s == " "

Preprocess by hand the small number of places where Raise does not begin on a newline

I have fixed by hand.

@dra27
Copy link
Member

dra27 commented Jun 30, 2020

What I meant with the pre-processing is that if you ensure by hand that "Raise" is always on a newline then you won't have any cases of "text Raise"

@dra27
Copy link
Member

dra27 commented Jun 30, 2020

Does I modify failwith description or it is obvious ?

I don't understand - where?

@dra27
Copy link
Member

dra27 commented Jun 30, 2020

Oh, do you mean the description of Stdlib.failwith (and presumably Stdlib.invalid_arg). I would leave those as they are, yes, given that those functions are precisely about raising an exception.

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

This is a really nice set of changes! I went through everything one last time - there are the last few smallest nits (and, as there were some, there is a typo which I spotted but which was nothing to do with the change)

This feels highly worth of a Changes entry, if you want to add an entry in the Changes file in the Standard Library section? I'm happy to rebase this on to 4.11, so it can go in the Standard Library section of the 4.11 changes.

stdlib/listLabels.mli Outdated Show resolved Hide resolved
stdlib/bytes.mli Outdated Show resolved Hide resolved
stdlib/stdlib.mli Outdated Show resolved Hide resolved
stdlib/stdlib.mli Outdated Show resolved Hide resolved
stdlib/stream.mli Outdated Show resolved Hide resolved
stdlib/stringLabels.mli Outdated Show resolved Hide resolved
@Et7f3
Copy link
Contributor Author

Et7f3 commented Jun 30, 2020

grep "Raise" $(find . -name "*.mli") show me others in otherlibs and in many places.

I'm happy to rebase this on to 4.11

I have already rebased onto trunk.

@dra27
Copy link
Member

dra27 commented Jun 30, 2020

grep "Raise" $(find . -name "*.mli") show me others in otherlibs and in many places.

Please feel very welcome to do a follow-up PR, although this one is now quite large and I'd prefer not to have to read them all again!

I'm happy to rebase this on to 4.11

I have already rebased onto trunk.

That's correct - this will be merged to trunk, I was talking cherry-pick it all for OCaml 4.11 (i.e. don't worry!)

Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@Et7f3
Copy link
Contributor Author

Et7f3 commented Jun 30, 2020

Only read the two last commit (I just moved two lines in the last commit)

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Sorry - I ran the diff through the patdiff tool which has better context display than GitHub. I promise this is the last tranche!

stdlib/bigarray.mli Outdated Show resolved Hide resolved
stdlib/float.mli Outdated Show resolved Hide resolved
stdlib/float.mli Outdated Show resolved Hide resolved
stdlib/int32.mli Outdated Show resolved Hide resolved
stdlib/lazy.mli Outdated Show resolved Hide resolved
stdlib/listLabels.mli Outdated Show resolved Hide resolved
stdlib/listLabels.mli Outdated Show resolved Hide resolved
stdlib/nativeint.mli Outdated Show resolved Hide resolved
stdlib/scanf.mli Outdated Show resolved Hide resolved
Et7f3 and others added 2 commits June 30, 2020 12:16
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@dra27 dra27 changed the title fix formatting comment for ListLabels.mli Consistently use @raise tags in Stdlib docs Jun 30, 2020
@dra27 dra27 assigned dra27 and unassigned gasche Jun 30, 2020
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

I've checked through the HTML output as well - this is a very nice set of updates to the standard library documentation, thank you!

@dra27 dra27 merged commit c4851b0 into ocaml:trunk Jun 30, 2020
dra27 pushed a commit that referenced this pull request Jun 30, 2020
@Et7f3 Et7f3 deleted the patch-1 branch June 30, 2020 13:17
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.

None yet

4 participants