Keep documentation comments in structures with no other items (MPR#7701 cont.)#1693
Keep documentation comments in structures with no other items (MPR#7701 cont.)#1693gasche merged 4 commits intoocaml:trunkfrom
Conversation
| (** The comment for variable x. *) | ||
| val mutable x : int | ||
|
|
||
| (** The commend for method m. *) |
tarptaeya
left a comment
There was a problem hiding this comment.
There is a typo, please correct it
| (** The comment for type my_record *) | ||
| type my_record = { | ||
| val foo : int ; (** Comment for field foo *) | ||
| val bar : string ; (** Comment for field bar *) |
There was a problem hiding this comment.
This was grossly invalid. Is it possible that using one of the {caml_example} environments would have caught the mistake earlier? If applicable in this case, I would recommend using them as part of this PR -- if we fix, let's fix long-term. cc @Octachron who may be able to comment on applicability.
There was a problem hiding this comment.
In this case, the code example is a signature; this would require a small tweak to caml_tex2.ml to add an option to wrap code example inside a module type declaration.
| module Manual : | ||
| sig | ||
| [@@@ocaml.text | ||
| " Special comments can be placed between elements and are kept\n by the OCamldoc tool, but are not associated to any element.\n @-tags in these comments are ignored."] |
There was a problem hiding this comment.
is there a way to use line breaks in the expect result here that is portable and correct, and increases readability of the test file?
There was a problem hiding this comment.
Without changing how -dsource prints things, any such change would be undone when promoting the result of the test.
There was a problem hiding this comment.
So, alternative question: is there a way for -dsource to print this in a nicer way? (This would be a separate PR.)
There was a problem hiding this comment.
I think that
" foo
bar"
is not portable due to \n vs. \r\n concerns,
" foo\n\
bar"
does not work as spaces at the beginning of the line are ignored, but that one could use
" foo\n \
bar"
pushing indentation of a line on the previous line.
Octachron
left a comment
There was a problem hiding this comment.
The core changes implement the right fixes. I have some minor existential interrogations on the docstring test but nothing that really matters.
|
|
||
| module M = struct | ||
|
|
||
| (** foo1 *) |
There was a problem hiding this comment.
It would be nice to capture the warning 50 raised by those unattached documentation comments, but this would require patching expect_text.ml . However, it could be nice to explicit this point here
(** foo1: this comment is unattached*) .
| " The comment for type my_record "] | ||
| class my_class = | ||
| object | ||
| inherit cl[@@ocaml.doc |
There was a problem hiding this comment.
Would it makes sense to permute the definition of my_class and my_class_type and inherit my_class_type here? After fixing the type of the method m, it would avoid the unbound class error later.
There was a problem hiding this comment.
I don't think you can inherit from a class type there.
There was a problem hiding this comment.
You are right of course, I was thinking of the signature part.
| |}] | ||
|
|
||
| (***********************************************************************) | ||
| (* Empty doc comments (GPR#548) *) |
There was a problem hiding this comment.
By pure curiousity, is there a reason beyond personal preference for having a single file with multiple parts rather than multiple files? And with a single file, would it not be better to merge the docstrings folder with the parsetree one, in order to reduce the number of tests folder in the test suite?
There was a problem hiding this comment.
Nothing beyond personal preference. I tend to like single test files that act as a form of documentation for how a particular feature behaves. Since I wanted to switch the "empty" tests to use a -dsource expect test anyway, I figured I might as well put them with the others.
And with a single file, would it not be better to merge the docstrings folder with the parsetree one, in order to reduce the number of tests folder in the test suite?
Yeah, that's probably a good idea.
|
|
||
| type s | ||
| (** bar1 *) | ||
| (** bar2 *) |
There was a problem hiding this comment.
Same comment as above: (** bar2: this comment is unattached *)?
|
Ok I moved the docstring tests into |
|
@Octachron : assuming a Changes entry was added, are you now satisfied with the state of this PR and can it be merged? Also, I'm wondering if this bugfix could/should be backport to the 4.07 branch. Any opinion? |
|
Yes, I am totally satisfied with the current state. Sorry for not being explicit. Backporting seems fine but not urgent to me since there is a workaround for the issue. |
|
A pure side comment. It would be nice for people reading the pull requests if the title of something like this was:
which make it easy for a casual reader to remember if is was a Mantis issue they are following. |
Keep documentation comments in structures with no other items (MPR#7701 cont.) (cherry picked from commit eb62d2e)
This stops us from dropping documentation comments in structures and signatures that have no other items in them. It also adds some more docstring tests and fixes some typos in the ocamldoc documentation.
This is a more complete fix than #1562, which only handled the case where the signature in question was an entire mli file.