docs icon indicating copy to clipboard operation
docs copied to clipboard

Update package.json scripts

Open XhmikosR opened this issue 2 years ago • 13 comments

  • remove build from linkinator script
  • switch to using --verbosity for linkinator

Signed-off-by: XhmikosR [email protected]

By submitting this pull request, I confirm the following: please fill any appropriate checkboxes, e.g: [X]

  • [x] I have read and understood the contributors guide, as well as this entire template.
  • [x] I have made only one major change in my proposed changes.
  • [x] I have commented my proposed changes within the code.
  • [x] I have tested my proposed changes, and have included unit tests where possible.
  • [x] I am willing to help maintain this change if there are issues with it later.
  • [x] I give this submission freely and claim no ownership.
  • [x] It is compatible with the EUPL 1.2 license
  • [x] I have squashed any insignificant commits. (git rebase)

Please make sure you Sign Off all commits. Pi-hole enforces the DCO.


XhmikosR avatar Jul 25 '21 11:07 XhmikosR

✔️ Deploy Preview for pihole-docs ready!

🔨 Explore the source changes: dd16f45e0ca14067a62c472d80df2353d353f641

🔍 Inspect the deploy log: https://app.netlify.com/sites/pihole-docs/deploys/60fd817b58a3f00007646729

😎 Browse the preview: https://deploy-preview-543--pihole-docs.netlify.app

netlify[bot] avatar Jul 25 '21 11:07 netlify[bot]

I disagree with the first point

  • remove build from linkinator script

We've added this after two or three cases where users ran npm test without any issues because they forgot to run build as a separate step. However, the CI reported errors and the initial confusion was there. To me - because the linkinator test is not meaningful without a preceding build - this is a logical addition.

DL6ER avatar Jul 25 '21 14:07 DL6ER

This is useless because npm t will fail if the build folder isn't present and 2, the build script runs needlessly twice on CI.

XhmikosR avatar Jul 25 '21 15:07 XhmikosR

Proof FYI:

C:\Users\xmr\Desktop\docs>npm t

> [email protected] test C:\Users\xmr\Desktop\docs
> npm run markdownlint && npm run linkinator


> [email protected] markdownlint C:\Users\xmr\Desktop\docs
> markdownlint-cli2 "**/*.md" "!**/node_modules/**"

markdownlint-cli2 v0.1.3 (markdownlint v0.23.1)
Finding: **/*.md !**/node_modules/**
Linting: 73 file(s)
Summary: 0 error(s)

> [email protected] linkinator C:\Users\xmr\Desktop\docs
> linkinator site --recurse --silent --skip "^(?!http://localhost)"

🏊‍♂️ crawling site
Error: The provided glob "site" returned 0 results. The current working directory is "C:\Users\xmr\Desktop\docs".
    at Object.processOptions (C:\Users\xmr\Desktop\docs\node_modules\linkinator\build\src\options.js:59:23)
    at async LinkChecker.check (C:\Users\xmr\Desktop\docs\node_modules\linkinator\build\src\index.js:32:25)
    at async main (C:\Users\xmr\Desktop\docs\node_modules\linkinator\build\src\cli.js:162:20)
npm ERR! code ELIFECYCLE
npm ERR! errno 1

XhmikosR avatar Jul 25 '21 15:07 XhmikosR

If you insist on having it there, I could probably refactor the scripts and update the CI script. But it's not that hard for people to obviously build the site beforehand...

XhmikosR avatar Jul 25 '21 15:07 XhmikosR

Yes, but if it has been built before the test would be running on an outdated version.

DL6ER avatar Jul 25 '21 16:07 DL6ER

It may not be hard, but remember, we need to make the barrier to entry to be as absolutely low as possible. If we have to wait longer for CI to run then that's the cost of how we operate.

People editing these docs and files likely have not ever used any of these tools before and may be using the GitHub interface to make their edits.

I understand the desire to have a lean and mean build process but I think we need to error on the side of caution and be mindful of who our submitters may be.

dschaper avatar Jul 25 '21 19:07 dschaper

I'll refactor the scripts later and add a start and a dev script which will watch for changes and should simplify the whole process.

XhmikosR avatar Jul 26 '21 05:07 XhmikosR

I appreciate it, thanks!

dschaper avatar Jul 26 '21 17:07 dschaper

So, this is a lot trickier than I thought :/

mkdocs serve doesn't write the files to the disk (my guess is so that it's faster when developing) so we can't use it, but we should use it.

The other solution would be to have a watch script which would call the npm test script after the mkdocs build but this is definitely a worse developing behavior since it's a lot slower.

Another solution would be to have the build script as a pretest script; this would automatically build when one runs npm test.

Let me know your thoughts.

XhmikosR avatar Aug 04 '21 13:08 XhmikosR

Thank you for taking a look at it and working on it.

I think the pretest step would solve the issues, DL would need to confirm that works for him as well. If we properly document that npm test is the step that needs to be done then I think we would cover the cases we need to cover.

dschaper avatar Aug 04 '21 14:08 dschaper

Another solution would be to have the build script as a pretest script; this would automatically build when one runs npm test.

So what would be the difference in the end?

DL6ER avatar Aug 04 '21 18:08 DL6ER

That the linkinator script won't affect the build and only the npm test script will have build automatically run when it runs.

XhmikosR avatar Aug 04 '21 18:08 XhmikosR