Uh oh!
There was an error while loading. Please reload this page.
Doc normalize markdown reference links - #29558
Conversation
There was a problem hiding this comment.
This wasn't matching the reference link because of the extra text
There was a problem hiding this comment.
Only the changes for the "Class" column were flagged by remark-lint. Replaced the <code> for consistency
There was a problem hiding this comment.
These were rendering as links in the heading because of the reference link [encoding]
nschonni
commented
Sep 24, 2019
@Trott I used the web to resolve the conflicts. Is there a better way to split this to make it easier for reviewers? |
I suppose if you want smaller change sets for easier review, you can either do one file per branch/PR or one type of change per branch/PR. @nodejs/documentation Anyone want to review this as-is? It seems OK to me. I understand why we should escape |
I don't have a problem with the changes being across multiple files, but I am having trouble figuring out what the specific changes are, and why. I edited the description to add the commit message description in, not sure why github didn't do that itself. For example, the unescaped Some of the changes seem to be literally malformed URLs, those could maybe be their own commit, and are obviously a good idea. The adding of empty In general, we know the markup is right when the output is right, so making things "right" with no tool support to maintain that "rightness" is fragile. If we really do require these markup changes, then tool support is needed to enforce them. It sounds like @nschonni you are relying on some kind of tools opinion about what correct markdown should be, and making the changes it suggests? |
nschonni
commented
Oct 1, 2019
These are flagged by remark-lint when using the "recommended" preset in addition to the current remark-node-preset, which has an upstream PR |
899413e to
eb0fedaComparenschonni
commented
Oct 1, 2019
I've broken up the commits a little further to make it easier to review the individual types of changes |
bfff527 to
0622d20CompareTrott
commented
Oct 1, 2019
@nodejs/collaborators This could use some reviews. |
BridgeAR
commented
Oct 1, 2019
@vsemozhetbyt do you know how much our tooling has to fix currently to get our links to work as expected and would this improve our situation? @nschonni I think it would be great if this PR could be split into multiple ones. That way they would probably be signed-off much faster. Especially since some parts might be more controversial to some than others. I definitely see the value in most of the change but there are a few which we might not want to change right now. I think splitting out 3899b8c and f4f7384 would be enough, since those two seem most controversial at the moment and make up a big part of this PR. |
nschonni
commented
Oct 1, 2019
I'm fine with splitting it up, but I'm wondering how that's done here. Close this PR and open new PRs for each commit, or leave the first commit and submit new ones for the others? |
@nschonni I would roughly do the following: # Get the latest upstream code in your fork
git checkout master
git fetch upstream
git pull upstream/master
git push origin
git checkout -b add-explicit-brackets-to-docs
git cherry-pick 3899b8c
git push -u origin add-explicit-brackets-to-docs
git checkout master
git checkout -b escape-brackets-in-docs
git cherry-pick f4f7384
git push -u origin escape-brackets-in-docs
git checkout doc--normalize-markdown-reference-links git rebase -i origin/master
# Here you just remove the two commits that you cherry picked earlier
git push --force-with-lease |
vsemozhetbyt
commented
Oct 2, 2019
@BridgeAR Unfortunately, I've lost the track of our doc tooling system since we transfer it to the |
Reference links were used in the document, but not define at the end.
It gets confused as a reference link otherwis
0622d20 to
1720122Comparenschonni
commented
Oct 2, 2019
OK @BridgeAR I've split this out as recommended |
BridgeAR
commented
Oct 2, 2019
@nschonni thanks a lot! |
BridgeAR
commented
Oct 2, 2019
@nschonni are you planning on including the here used rules to our doc linter as well? Without that it will be hard to prevent any of these to be reintroduced. |
nschonni
commented
Oct 2, 2019
They're currently re-ignored in nodejs/remark-preset-lint-node#21, but can be enabled once these land |
Trott
commented
Oct 2, 2019
Trott
commented
Oct 2, 2019
Landed in 344c5c4...420d4e4 |
PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reference links were used in the document, but not define at the end. PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
It gets confused as a reference link otherwis PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reference links were used in the document, but not define at the end. PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
It gets confused as a reference link otherwis PR-URL: #29558 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Use explicit trailing
[]for reference markdown links.Escape brackets in titles that get flagged by remark-lint.
Convert old changlogs SHA links to match newer format.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes