Skip to content
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

Add checks for broken links #77

Open
jenweber opened this issue Dec 6, 2018 · 6 comments
Open

Add checks for broken links #77

jenweber opened this issue Dec 6, 2018 · 6 comments

Comments

@jenweber
Copy link
Contributor

jenweber commented Dec 6, 2018

As noted in #73, to add checks for broken links using remark, we need to run Node 8 in our CI.

After #76 is merged, we can add it!

cc @maxwondercorn

@maxwondercorn
Copy link
Contributor

I've run into a quandary with checking broken links. There is one link that will always fail in CI and that is http://localhost:4200. The linter has no method for whitelisting or excluding urls to check.

By default Remark only issues warnings, so the current lint process doesn't throw an error if the linter finds anything. If we markdown lint errors we were't catching them in CI. Remark will trigger an error if the --frail option is used.

Normally, I'd say we should trigger an error to catch issues before accepting a change. In this case, we'd never pass CI because of the "dead" localhost link.

I see a few options here:

  • Change the guide writing-addons/intro-tutorial.md to not have the link. Not the best idea because it adds value to the guide
  • Ask contributors to have a clean lint and maintainers review the Travis log as part of the review process. Not hard to do but does add burden to the maintainers
  • I'm going to raise an issue in the plugin's repo to see if exclusions can be added. I've looked at all the dependencies down to the module that does the checking - no exclusion options
  • other options?

There may be some method to stub this but I'm a neophyte when it comes to travis-ci

In any case, we should add something to CONTRIBUTING.md asking contributors to run the listing tests and clean up the warnings before submitting a PR. I'll add something when I have a PR.

@mansona
Copy link
Member

mansona commented Dec 6, 2018

@maxwondercorn the alternative is that we could do the link checking ourselves 🤔 We're already checking internal links with the guidemaker link checker (which is essentially just a port of @jenweber's link-checking code on the Ember Guides)

We could expand that link checker to also check outbound links 🤔 What kinds of links are we looking to check exactly? do we have an example of something that was broken and should have been caught in CI?

@maxwondercorn
Copy link
Contributor

@mansona The existing checks in node-tests are only for relative links in the markdown. The link checker I'm referring to will catch outbound links as you indicated.

You gave me an idea 🤔 I can add a script command just to lint dead links. It can be manually run as needed. We can have the other linting generate errors to catch inconsistencies.

@jenweber
Copy link
Contributor Author

jenweber commented Dec 6, 2018

I think remark would prob welcome the whitelisting PR. The remark ecosystem is on the upswing and I know one of the maintainers.

@maxwondercorn
Copy link
Contributor

PR #79 closed per discussion about dead link checking - needs to be automated from day one.

@mehulkar
Copy link
Contributor

mehulkar commented Apr 3, 2020

@jenweber @locks I think this can be closed. npm run test:node checks for broken links using guidemaker, from what I can tell

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

No branches or pull requests

4 participants