search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.58k forks source link

lint request: enforce recommended dartdoc structure #57907

Open pq opened 5 years ago

pq commented 5 years ago

In https://github.com/flutter/flutter/pull/27791#discussion_r255772365, @goderbauer wrote:

That a dartdoc should start with a one-sentence summary as its own paragraph is something I have to frequently point out in code reviews - so I'd prefer it if things like these could be pointed out automatically.

👍

Let's talk details.

  1. Enforcing a summary statement lines up w/ this advice from Effective Dart and I think we should just do it: https://www.dartlang.org/guides/language/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph

Other thoughts?

goderbauer commented 5 years ago

Yes, yes, yes! Please!

srawlins commented 5 years ago

One thing I'd like to guard against: Pythnon's linter (one of them) enforces a one line summary which I think is a disaster. It limits the summary to something like 75 characters, yuck. One sentence is the Effective Dart recommendation, and we should make sure to stick to that.

I'm not volunteering to write the regex / Markdown parser for this 😁

pq commented 5 years ago

I'm not volunteering to write the regex / Markdown parser for this 😁

Hopefully we can just use the parser from package:markdown for this? Alternatively we could crib some bits from dartdoc?

pq commented 5 years ago

Or is that even over-thinking it? Can we just get away with something simple like

comment.split('\n\n');

?

goderbauer commented 5 years ago

How would you make sure that after splitting the first part is just one sentence though?

pq commented 5 years ago

Yeah. I realized that. A simple scanner might work but then maybe there are complex cases.

@jcollins-g: would it be easy for us to crib this logic from dartdoc?

EDIT: I was assuming there is such logic in dartdoc but I don't know actually why there would be. That is, from dartdoc's perspective, I believe the one liner is just derived from markdown parsed to html. Something like:

var html = markdownToHtml(docString);
var summary = HtmlParser(html).parse().body.children.first.innerHtml;

I'm guessing there's no validation that would notice

This is a bad summary. Because I didn't line-break.

Any more than it would wrongly split

Ms. Marvel is the name of several fictional superheroes appearing in comic books published by Marvel Comics.

Regardless, since performance will be a real issue I'm guessing that if we do want to do this we'd want to roll a regexp or simple scanner to match bad sentences. I have no idea how sloppy that'd be in practice or what the risk of false positive but it'd be pretty easy to do some testing on Flutter.

Anyway, it'd be interesting to get other thoughts here and if there is something we can get from dartdoc all the more awesome.

jcollins-g commented 5 years ago

There is logic here in dartdoc that divides the HTML output into two segments:

https://github.com/dart-lang/dartdoc/blob/9e5a57dd1d0826d5a59fd074d8eb2637466a5c17/lib/src/markdown_processor.dart#L874

As @pq surmised, we take the first node of generated HTML for the summary (modulo a bunch of dartdoc special casing and stripping of script tags, etc). We discussed offline and my recommendation for lints that are likely to produce decent results in dartdoc regardless of localization or personal preference would be:

I think narrowing it down to a "one sentence" summary is probably less general. Maybe a separate lint not enabled by default if we really want it?

pq commented 5 years ago

After chatting w/ @jcollins-g and @bwilkerson, I'm wondering if the following might not get us a lot of the way there:

  1. a lint that flags docs whose summary paragraph is longer than some reasonable length. Notice how MethodInvocation wraps below. We could flag that and suggest they break the doc into summary and details.

    image

  2. a lint that flags doc summaries that contain stuff that wouldn't render well (e.g., doesn't start w/ text, maybe contains links?)

@goderbauer: thoughts?

goderbauer commented 5 years ago

I've never really seen 2 as a problem, so not sure how useful that one will be...

For 1 I'd be curious what we'd consider a reasonable length to not run into Python's annoying limitation of "this has to be 80 chars or less"...

ICU has some heuristics to detect sentence breaks based on Unicode recommendation: http://userguide.icu-project.org/boundaryanalysis#TOC-Sentence-Boundary Maybe that can be useful?

pq commented 5 years ago

I've never really seen 2 as a problem, so not sure how useful that one will be...

I'm not sure either to be honest but then the Flutter docs have benefited from extraordinary care. This may be more valuable to more slap-dash efforts.

For 1 I'd be curious what we'd consider a reasonable length to not run into Python's annoying limitation of "this has to be 80 chars or less"...

Me too. @jcollins-g: I'm curious if you have a gut sense for this?

Thanks for the ICU link. It would be interesting to see how many false positives we'd get with their heuristics. Would that there was a dart package implementation so we could do some quick testing!

jcollins-g commented 5 years ago

I've never really seen 2 as a problem, so not sure how useful that one will be...

I'm not sure either to be honest but then the Flutter docs have benefited from extraordinary care. This may be more valuable to more slap-dash efforts.

For 1 I'd be curious what we'd consider a reasonable length to not run into Python's annoying limitation of "this has to be 80 chars or less"...

Me too. @jcollins-g: I'm curious if you have a gut sense for this?

As above, my gut sense is "two lines" -- of any length. ~150 characters might also work.

Thanks for the ICU link. It would be interesting to see how many false positives we'd get with their heuristics. Would that there was a dart package implementation so we could do some quick testing!

ditman commented 2 years ago

I want this sooo badly. Can it please also enforce that the first line ends in a "."?

srawlins commented 2 years ago

Don't expect this any time soon.

A rule was attempted in https://github.com/dart-lang/linter/pull/2148 and was deemed too slow. The comments over there highlight a lot of the dragons to be found in an implementation. All of that being said, contributions welcome! I'd love this rule too :D. This is one of the most-referenced Effective Dart rules in internal readability reviews.

ditman commented 2 years ago

@srawlins that rule seems way more complex than what I'd be (very) happy with ((I'm a simple man :P)).

I'm not suggesting to validate the whole doc block to add periods at the end of each paragraph or anything, just that the summary line of the DartDoc comment (the first/only one) ends with a period :P

Something like this:

I wouldn't even bother with the length of the line, unless very long lines end up looking terrible in the processed DartDoc output!

(Checkstyle allows to configure the "period" character at the end, which is cool for programmers writing comments in japanese, though)

((PS: This looks interesting, I might give it a shot!))

srawlins commented 2 years ago

I think that is a separate request from this one. The Effective Dart guideline requested here is: https://www.dartlang.org/guides/language/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph

You might be looking for https://github.com/dart-lang/sdk/issues/58188 or https://github.com/dart-lang/sdk/issues/57202.