Skip to content

Conversation

trickeydan
Copy link
Contributor

When these were recently removed in 1add520, the reason cited was that it broke the link checker. I have fixed this by excluding those links from the checker

package.json Outdated
"spellcheck": "mdspell --en-gb --report --ignore-acronyms --ignore-numbers --no-suggestions 'content/**/*.md'",
"broken-link-local": "blcl --recursive --ordered --exclude-external ./public/",
"broken-link-all": "blcl --recursive --ordered ./public/",
"broken-link-all": "blcl --recursive --exclude https://github.com/sourcebots/docs/tree/master/content/* --ordered ./public/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could we have an inline comment explaining the exclusion.

Meta: this will likely conflict with #117.

Copy link
Contributor Author

@trickeydan trickeydan Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see above note about @RealOrangeOne's comments in 1add520

I am aware that this will likely conflict, but am not sure how best to resolve this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the reason for the exclusion; I'm asking for a comment so that it's clear to all future readers of the code.

Please bear in mind that future readers won't have to hand the comments on this PR or the original commit which removed the functionality. While they could git blame, the chances are that they'd have to chase several layers of commits to find the rationale (and then they'd need to work backwards from "Add Github Edit Links" to this PR and onwards from there). Adding a comment now is easy and makes things clearer for everyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed the word inline and mistook the context of your request.

Fixed in 9d037ff

package.json Outdated
"spellcheck": "mdspell --en-gb --report --ignore-acronyms --ignore-numbers --no-suggestions 'content/**/*.md'",
"broken-link-local": "blcl --recursive --ordered --exclude-external ./public/",
"broken-link-all": "blcl --recursive --ordered ./public/",
"//": "Github links below are excluded to prevent breaking the link checker for the Edit in Github urls, when no such page exists in master",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eurgh, does npm's JSON parser really not support comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@RealOrangeOne RealOrangeOne Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i never saw this hack. I'd rather remove this thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 4434fd3

Copy link
Member

@RealOrangeOne RealOrangeOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz remove attempt at a comment

Revert "Add explanatory comment on exclusion of certain github links from link checker"

This reverts commit 9d037ff.
@PeterJCLaw
Copy link
Contributor

Hnng. Why is the JavaScript ecosystem so crap.

This is a difficult one. I really want there to be more help to future readers of the package.json about why certain things are ignored than the minimal commit messages we have here. However I agree the that comment hack is really nasty.

Is there another way that we can configure the exclusions? Perhaps a config file for the tool? Alternatively, could we break that command out to a script (in the scripts directory) and have the comments there?

@RealOrangeOne
Copy link
Member

The tool has no config, although perhaps extracting it out into scripts/test.sh might be better, as we can have tests there. Obviously you'll need to expand path with node_modules/.bin/

@RealOrangeOne
Copy link
Member

wrong button -_-

@RealOrangeOne RealOrangeOne reopened this Mar 8, 2018
@PeterJCLaw
Copy link
Contributor

PeterJCLaw commented Mar 8, 2018

I think pulling out a ./scripts/check-all-links (with suitable $PATH munging) is reasonable. If we pulled out all the testing things to a single script I'd be in favour of removing them from the package.json to avoid confusion.

@trickeydan
Copy link
Contributor Author

As mentioned yesterday at the tech day, the scripts are out of scope of this PR.

@RealOrangeOne
Copy link
Member

It looks like due to a merge issue, farnel has been removed from the exclusion list. Restoring this should fix the tests

@trickeydan trickeydan dismissed RealOrangeOne’s stale review March 26, 2018 09:51

Script to check links is out of scope for this PR.

@trickeydan trickeydan mentioned this pull request Mar 26, 2018
@trickeydan trickeydan merged commit 308b685 into master Mar 26, 2018
@trickeydan trickeydan deleted the add-edit-links branch March 26, 2018 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants