-
Notifications
You must be signed in to change notification settings - Fork 394
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
tests: links: misc fixes #1067
tests: links: misc fixes #1067
Conversation
@@ -1,3 +1,4 @@ | |||
#!/usr/bin/env bash | |||
(find pages/ content/docs/ src/ .github/ -name '*.md' -o -name '*.js' && ls *.md *.js) \ | |||
repo="$(dirname "$(realpath "$(dirname "$0")")")" | |||
(find "$repo"/pages/ "$repo"/content/docs/ "$repo"/src/ "$repo"/.github/ -name '*.md' -o -name '*.js' && ls "$repo"/*.md "$repo"/*.js) \ |
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.
does it fail if one of the directories does not exist?
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.
no - tested (exit status still zero). I presume you're talking about the warning
find: ‘...dvc.org/pages/’: No such file or directory
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.
yes, would be good to catch changes in the docs structure if it's just a matter of adding some flag
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.
I assume pages
would be generated if the site was built. Are you saying it should be removed?
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.
no, no ... I'm not even sure if we need to test generated stuff .. just saying that next time someone decides to move /content
to something like /static/content
would be nice to detect that change ... again, not a big deal
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.
Right. Sounds like something for a different issue. Though it sounds like doing something like a directory move leaved the onus on the mover to update all tests.
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.
like a directory move leaved the onus on the mover to update all tests
correct! but it's better for tests to start failing.
Useful for large PRs
ready to merge? :) |
sed