Fix bugzilla 14413: Spurious newline in ddoc JSON output for multiple…

[Abscissa]

… successive line doc comments

Fixes: https://issues.dlang.org/show_bug.cgi?id=14413

When one doc comment ends, and the next doc comment starts on exactly the same or the next line, no extra newline (ie, paragraph break) is added between them when they're merged by Lexer::combineComments.

[Abscissa]

I have no idea why the autotester is failing this on Win32. It's saying "Error - file 'lexer.c' contains trailing whitespace", but the file very clearly doesn't (aside from the standard trailing \n):

https://github.com/Abscissa/dmd/blob/issue-14413/src/lexer.c#L2504

https://github.com/D-Programming-Language/dmd/pull/4745/files#diff-92587e94259bcadd0c463ae6237c5ecfL2447

[yebblies]

Trailing whitespace.

[Abscissa]

Oh, it didn't give a line number so I thought it meant "at the end of the entire file". Never seen indentation in a blank line be an issue before. None of my editors eliminate or highlight that sort of thing by default. Will fix.

[CyberShadow]

Looks good! Though in some places we need to replace newline-delimited lists with proper DDoc ($(UL ...)), mainly core.cpuid (for example see web/library-prerelease/core/cpuid/prefer_athlon.html in autotester results).

[jacob-carlborg]

I suggest cleaning up the commits.

[MartinNowak]

Please use a book, or even better a NewParagraph.yes/no enum flag .

[MartinNowak]

Why does your change only affect the json output?

[Abscissa]

I suggest cleaning up the commits.

Done

Please use a bool,

Done

Why does your change only affect the json output?

Hmm? It affects -D's html output for me, in addition to the json output. Try it on this:

///This should
///be one
///paragraph.

///Separate paragraph.
void a() {}

[yebblies]

You can see the full doc changes here: http://dtest.thecybershadow.net/results/d626300021917249181ec1434e74b090e80e1087/a581d0e5bfd08d84722ed66ca73e14f21071cf3c/

[MartinNowak]

Mmh, half of the doc changes are fixes, the other half now has broken formatting.
We'd at least need someone to fixup druntime/phobos before we can pull this. Any estimate on the impact for other projects? I don't use /// for multiline doc comments myself.

[Abscissa]

Mmh, half of the doc changes are fixes, the other half now has broken formatting. We'd at least need someone to fixup druntime/phobos before we can pull this.

Many of them also seem to be semi-arbitrary judgment calls which could work either way. I've mainly left those alone.

I've make PRs to fixup druntime/phobos: dlang/phobos#3722 and dlang/druntime#1408

Any estimate on the impact for other projects? I don't use /// for multiline doc comments myself.

I really doubt it would be a problem. (I do use /// for multiline doc comments). If you're using /// for multiline doc comments, then chances are you're already using a blank line as a paragraph break anyway. If you're not, then it already looks wrong in the code anyway (or, as was occasionally the case of phobos/druntime., you may be forgetting to use proper UL/LI list macros where appropriate). Ex:

/// Hello, this function does some
/// cool stuff.
/// This is really supposed to be
/// another paragraph?
/// It looks wrong even in here,
/// doesn't it?

vs:

/// Hello, this function does some
/// cool stuff.
///
/// This really is supposed to be
/// another paragraph.
///
/// It looks correct even in here,
/// doesn't it?

I think the fact that this has already caught "spurious newline" formatting errors in both druntime and phobos docs is a good sign that this is worthwhile. Plus, the very few breakages that did occur (not counting cases where it can work fine either way, I'd argue those aren't actual "breaking") were things that were arguably (IMO) already written wrong in the doc comments anyway. And even those were mostly in the curl bindings which really needs it's doc comments cleaned up a bit anyway.

[Abscissa]

Jesus, it broke again? Getting tired of this...

[Abscissa]

Rebased again.

[Abscissa]

Would it be bad form to ping this?

[MetaLang]

More or less the only way to get something merged is to constantly hound people

[Abscissa]

This has been collecting dust since DMDFE was in C. I'll fix the merge failures that keep cropping in whenever someone's interested in actually merging. I'm done babysitting an endlessly ignored PR in the meantime.

[Abscissa]

This has been rotting in limbo for about a year, can it get some attention?

[Abscissa]

Again, anytime somebody's interested in actually merging, I'll be glad to cleanup any conflicts and failed checks. Just sayin'. Hello? Is this thing on? tap tap tap...This is why I don't normally contribute to the base dlang projects anymore.

[andralex]

@adamdruppe of course! I don't have the time to get into the PR, just skimmed the chat. Yes, if it's appropriate please pull.

[CyberShadow]

Ok, crazy heretical idea here, but it's all getting stuck inside a <p class="para">...</p> right? Just make the "paragraph break" be </p><p class="para">.

Pretty sure we tried that at some point... it didn't work.

[andralex]

@CyberShadow I just tried it and runs of <p></p> are also merged together. So we should be able to generate paired <p>s. Cool!

[andralex]

@CyberShadow actually I think it can be made to work. The trick is to insert a <p> at the beginning of the doc and a </p> at the end of the doc.

Overall: let's focus on getting beautiful documentation first, less aggravation to doc writers (e.g. no more need for the silly $(P ...)s) second, and generating paired <p>s third. The worst we can do is debate this to death and conclude no progress can be made. The second worst is develop a really convoluted solution for a small benefit. Thanks!

[CyberShadow]

@CyberShadow actually I think it can be made to work. The trick is to insert a <p> at the beginning of the doc and a </p> at the end of the doc.

No, that's exactly what we tried.

Unfortunately I don't remember what went wrong :(

[andralex]

@CyberShadow Then let's forego pairing of <p>s until we have a solution. The priority is formatting and doc writer experience. Thanks!

[andralex]

looked over the code and looks reasonable and quite what I'd expected. All tests pass, too. @CyberShadow @adamdruppe @MartinNowak anything stopping this from getting merged?

[CyberShadow]

Hmm, the documentation tester didn't report in. Let me look at that.

[CyberShadow]

Hmm, the documentation tester didn't report in. Let me look at that.

Found the problem - we have over 100 open pull requests open for DMD, and this pull request is below the 100 line. GitHub has a limit of 100 items per page in API calls. I may have been a bit too hopeful that we'll never need to test more than 100 open pull requests per repo :)

Implementing paging real quick.

[CyberShadow]

Done!

Though, unless someone did something about the error in std.random, the build will fail as before.

[andralex]

@adamdruppe yah, now I remember my old PR addressed this matter.

@CyberShadow thanks!

[JackStouffer]

Though, unless someone did something about the error in std.random, the build will fail as before.

I would love to, but the error isn't reproducible locally.

[CyberShadow]

I would love to, but the error isn't reproducible locally.

Note that it occurs in 2.073.0's Phobos, not master. It's possible that it will (or can only) be fixed by a 2.073.1 release.

If you have time to dig into this, let me know and I'll give you access to the machine/account.

[CyberShadow]

I would love to, but the error isn't reproducible locally.

BTW, how did you try to reproduce it?

I haven't tried but digger build --with=website should do mostly the same thing as the documentation tester.

[CyberShadow]

I haven't tried but digger build --with=website should do mostly the same thing as the documentation tester.

OK, that does work on my machine :/

Perhaps it could be something like a difference in gcc versions exposing a DMD bug or something...

[adamdruppe]

On Fri, Feb 10, 2017 at 05:31:17AM -0800, Andrei Alexandrescu wrote: I can tell that if we go with `<br>` as a paragraph separator that definitely won't work. `<br>` does not enjoy the necessary treatment.

It does not have special status, that's just the way margins work in HTML and CSS, adjacent margins are collapsed into one. The exact status of `<p>` vs `<p></p>` is unclear - traditional HTML as well as HTML5 do not require the closing tag, they just see the opening one as start a new paragraph and there's various conditions that implicitly close them. XHTML does require it and I prefer it (it is indeed easier to parse and validate) but xhtml isn't required for the web itself...

[adamdruppe]

On Fri, Feb 10, 2017 at 06:24:47AM -0800, Andrei Alexandrescu wrote: anything stopping this from getting merged?

no objection in my eyes, it looks good to me.

[CyberShadow]

anything stopping this from getting merged?

I would like to see a doc tester diff...

[jacob-carlborg]

You might be able to hack that up with css

I would rather not output the macro to begin with.

[JackStouffer]

If you have time to dig into this, let me know and I'll give you access to the machine/account.

If it's not an issue with Phobos I don't know where I'd start :/. I don't really use ddoc.

[CyberShadow]

If it's not an issue with Phobos I don't know where I'd start :/. I don't really use ddoc.

Well, looking at the logs, it is an issue that's at least related to Phobos, since the errors occur there. I don't know what the root cause is, though...

[CyberShadow]

I would like to see a doc tester diff...

Well, we got diffs back, and it looks great!

What is the significance of the codecov failures? Would merging with those failures break master?

@Abscissa Could you please rebase and squash the fixups into one commit?

[Abscissa]

Rebased and squashed

[CyberShadow]

And so the squeaky wheel got its oil, eh :) Thanks!

[andralex]

woo-hoo what a victory for the process and the folks who made this happen!

[Abscissa]

Awesome, thanks all!