-
-
Notifications
You must be signed in to change notification settings - Fork 383
Upgrade ddox #1891
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
Upgrade ddox #1891
Conversation
|
Thanks for your pull request, @wilzbach! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
08a291c to
9ddd84f
Compare
Finally got even the search working again. Should be good to go now - let's see what DAutoTest has to say. |
| - if (auto pp = cast(Package)pack.parent) | ||
| - return pp.lookup!Module(pack.name); | ||
| - return null; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's defined in ddox.inc.utils.dt which is now in this scope too.
dpl-docs/views/ddox.overview.dt
Outdated
| - moduleInfoRec(p); | ||
| - foreach( m; pack.modules ) | ||
| - if (!m.qualifiedName.startsWith("std.c.") && !m.qualifiedName.startsWith("core.stdc.") && !m.qualifiedName.startsWith("core.sys.")) | ||
| - auto qualifiedName = m.qualifiedName.to!string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is .to!string necessary? .startsWith should work with any string.
0fcfd05 to
8613e35
Compare
| ul | ||
| li | ||
| a(href="#{root_dir}spec/spec.html")Language Reference | ||
| a(href="#{root_dir}spec/spec.html") Language Reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newer Diet versions enforce a whitespace before the text.
| var disqus_shortname = 'vibe-d'; // required: replace example with your forum shortname | ||
| var disqus_developer = 1; | ||
| var disqus_identifier = #{"\\\"phobos-" ~ info.node.qualifiedName ~ "\\\""}; | ||
| var disqus_identifier = #{text(`"phobos-`, info.node.qualifiedName.text, `"`)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info.node.qualifiedName was converted to a range (and isn't a string anymore)
| block ddox.title | ||
|
|
||
| block navigation | ||
| include ddox.inc.utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heading has been moved in this auxiliary file: https://github.com/rejectedsoftware/ddox/blob/8ceb3815552765630e9fa0dece63f35837d416ee/views/ddox.inc.utils.dt
|
Sadly due to a change in diet-ng, ddox now produces compact HTML, so the entire diff is unusable.
|
8613e35 to
bd788e2
Compare
Thanks to @s-ludwig's fast reaction time, the PR got in and has been applied for this PR, s.t. the diff hopefully will be small. |
Sadly it's not due to other changes in the rendering of diet-ng |
Friendly ping ;-) |
IIRC the CI was failing at the time of your previous comment. Still, this does not look right: + <a id="MemoryOrder" class="["public"]" href="../core/atomic/memory_order.html">MemoryOrder</a> |
|
There are a lot of whitespace changes, which make it difficult to find the other kinds. Is it possible to split this into two or three PRs, one of which bumps past just the commits which introduce the whitespace changes? Or, more generally, one PR per ddox change which affects generated content would be even better. |
Tried to only bump diet-ng or vibe-d -> no changes. (this PR doesn't include the Any other ideas? |
|
We could of course make a PR that runs the output through some HTML formatter (e.g. "tidy", which is included in the Debian repositories), once for the current version and once for the new one.
Good catch, that is a silent behavior change in diet-ng vs the old implementation. I kind of lean towards keeping the semantics and fixing it in Ddox, tough (dlang/ddox#171). I'll try to get to fixing the recently discovered issues ASAP. |
-> #1940 (but it would be a lot easier if we find the problems directly).
I submitted both options as PRs: rejectedsoftware/diet-ng#41 and dlang/ddox#187. |
10f0b34 to
bd788e2
Compare
@s-ludwig we are still blocked on the Ddox regression for this update. Which PR do you prefer? |
bd788e2 to
91eb693
Compare
91eb693 to
33195fc
Compare
|
Okay I took one file in the DDox, applied
So imho everything looks good now and I think we are finally ready to upgrade. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯\(ツ)/¯
|
Nice, thanks for the effort. Gave it a shot myself, but it was quite tedious. |
This is WIP and at least gets dpl-docs compiling with ddox 0.16.
I opened this PR to (ab)use DAutoTest a bit - sorry for the noise.