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

Keep documentation comments in structures with no other items (MPR#7701 cont.) #1693

Merged
merged 4 commits into from May 5, 2018

Conversation

Projects
None yet
6 participants
@lpw25
Copy link
Contributor

lpw25 commented Apr 3, 2018

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.

(** The comment for variable x. *)
val mutable x : int

(** The commend for method m. *)

This comment has been minimized.

@Tarptaeya

Tarptaeya Apr 3, 2018

This must be comment, not commend

@Tarptaeya
Copy link

Tarptaeya left a comment

There is a typo, please correct it

@@ -494,8 +494,8 @@ type weather2 =

(** The comment for type my_record *)
type my_record = {
val foo : int ; (** Comment for field foo *)
val bar : string ; (** Comment for field bar *)

This comment has been minimized.

@gasche

gasche Apr 3, 2018

Member

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.

This comment has been minimized.

@Octachron

Octachron Apr 3, 2018

Contributor

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."]

This comment has been minimized.

@gasche

gasche Apr 3, 2018

Member

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?

This comment has been minimized.

@lpw25

lpw25 Apr 3, 2018

Author Contributor

Without changing how -dsource prints things, any such change would be undone when promoting the result of the test.

This comment has been minimized.

@gasche

gasche Apr 4, 2018

Member

So, alternative question: is there a way for -dsource to print this in a nicer way? (This would be a separate PR.)

This comment has been minimized.

@gasche

gasche Apr 4, 2018

Member

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
Copy link
Contributor

Octachron left a 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 *)

This comment has been minimized.

@Octachron

Octachron Apr 6, 2018

Contributor

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

This comment has been minimized.

@Octachron

Octachron Apr 6, 2018

Contributor

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.

This comment has been minimized.

@lpw25

lpw25 Apr 6, 2018

Author Contributor

I don't think you can inherit from a class type there.

This comment has been minimized.

@Octachron

Octachron Apr 6, 2018

Contributor

You are right of course, I was thinking of the signature part.

|}]

(***********************************************************************)
(* Empty doc comments (GPR#548) *)

This comment has been minimized.

@Octachron

Octachron Apr 6, 2018

Contributor

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?

This comment has been minimized.

@lpw25

lpw25 Apr 6, 2018

Author Contributor

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 *)

This comment has been minimized.

@Octachron

Octachron Apr 6, 2018

Contributor

Same comment as above: (** bar2: this comment is unattached *)?

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Apr 6, 2018

Ok I moved the docstring tests into docstring.ml in tests/parsing -- which already existed and had yet another docstring test. I've also updated the comments as suggested. I haven't fixed the example code to type-check because it's not needed and I would prefer to keep the code close to the example in the ocamldoc manual.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Apr 26, 2018

@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?

@Octachron

This comment has been minimized.

Copy link
Contributor

Octachron commented Apr 26, 2018

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.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented Apr 26, 2018

A pure side comment. It would be nice for people reading the pull requests if the title of something like this was:

Fix MPR#7701: "Doc comment dropped from empty .mli file"

which make it easy for a casual reader to remember if is was a Mantis issue they are following.

@dra27 dra27 changed the title Fix MPR#7701 Keep documentation comments in structures with no other items (MPR#7701 cont.) Apr 26, 2018

@gasche gasche merged commit eb62d2e into ocaml:trunk May 5, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

gasche added a commit that referenced this pull request May 5, 2018

Merge pull request #1693 from lpw25/fix-mpr7701
Keep documentation comments in structures with no other items (MPR#7701 cont.)

(cherry picked from commit eb62d2e)

gasche added a commit that referenced this pull request May 5, 2018

gasche added a commit that referenced this pull request May 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.