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

failing test script if there is lint in the code or in the schema #377

Open
wants to merge 2 commits into
base: v1.0.0
Choose a base branch
from

Conversation

antialias
Copy link
Contributor

@antialias antialias commented Apr 24, 2020

I recommend merging #378 first

@evanplaice
Copy link
Member

We should avoid OS-specific tooling (ex shell scripts, CLI utils). CI is pretty barebones as-is and may be changed to include Windows as well.

@antialias
Copy link
Contributor Author

antialias commented Apr 24, 2020

We should avoid OS-specific tooling (ex shell scripts, CLI utils)

Why? Banning shell scripts implies not using scripts in package.json at all - they are all run as shell scripts by npm.

Also, ubuntu-latest is declared as as the OS which runs the checks, so there's no reason to try to make all the OSes happy at CI time.

CI ... may be changed to include Windows as well.

Then let's decide now not use Windows to run our lint checks but since Windows runs bash natively now, they would probably work as is anyway.

@evanplaice
Copy link
Member

Why? Banning shell scripts implies not using scripts in package.json at all - they are all run as shell scripts by npm.

package.json scripts that execute Node scripts are OS independent

Then let's decide now not use Windows to run our lint checks but since Windows runs bash natively now, they would probably work as is anyway.

Windows Subsystem for Linux can run bash scripts. That's not the default CLI shell in Windows; it's Windows 10 only, and it requires extra setup. If you'd like, I can add windows to the OS matrix in the GH Actions config to demonstrate.


Why? Including unix-specific scripting blocks Windows-based users and contributors from being able to run the tests.

@antialias
Copy link
Contributor Author

package.json scripts that execute Node scripts are OS independent

Can you be more specific about what you want / don't want here? The executables referenced in scripts are declared in package.json so they get installed by npm install along with the rest of this project's devDependencies. Exceptions are diff and printf, which have been around in every unix-like environment for decades.

Aside from "being OS independent" (and this PR maintains that for any unix-like env), do we not want to care about PRs introducing lint into the schema? Happy to go with another solution if you can recommend something.

These scripts are only intended for CI anyways, which is well-defined as ubuntu in the github actions that you set up, so I don't understand why we need to start checking for lint on windows all of a sudden.

@evanplaice
Copy link
Member

evanplaice commented Apr 24, 2020

Use Case:

A contributor (who uses Windows) is working on a PR. The PR has a mistake (ie linting error) so the CI fails as expected. How does that contributor identify what went wrong? That's why every script run on CI should also be able to run locally.

Aside from "being OS independent" (and this PR maintains that for any unix-like env), do we not want to care about PRs introducing lint into the schema?

The CI and CD checks are virtually identical. CI just breaks the preversion checks out into separate steps (ex test, lint) so it's easier to identify what failed. This is intentional, any PR that gets approved should be able to be merged in with no additional work.

Happy to go with another solution if you can recommend something.

An alternative should be JS-only. I'd probably go with a tool that uses or works like ESLint for JSON.


Personally, I haven't used a Windows for 5 years; even then it was only on work machines b/c I was required to. I would loooove if Windows just ceased to exist one day. With that said, not all devs are ready, willing, or able to ditch Windows.

@antialias
Copy link
Contributor Author

A contributor (who uses Windows) is working on a PR. The PR has a mistake (ie linting error) so the CI fails as expected. How does that contributor identify what went wrong? That's why every script run on CI should also be able to run locally.

Well the way I usually discover why CI failed is by looking at the output posted on the nifty github checks checks that you set up. I can view this with my test account that is not a member of the jsonresume team, so it will work for anyone.

Screen Shot 2020-04-24 at 6 53 15 PM

https://github.com/jsonresume/resume-schema/pull/377/checks?check_run_id=616829721

@antialias
Copy link
Contributor Author

antialias commented Apr 25, 2020

Happy to go with another solution if you can recommend something.

I'd probably go with a tool that uses or works like ESLint for JSON

I did. Here's what's being used and why:

  • eslint-plugin-json, an eslint plugin that checks JSON for validity (sadly, it does not order the keys nor enforce pretty printing, which is why I also added)
  • jsonlint-cli sorts the json and keeps it pretty-printed.

Both of these tools are implemented in javascript and have node APIs that can be called from within a .js file if we really want to go all-js, but calling them in the right sequence and failing in the correct way is a couple orders of magnitude more complicated when done from javascript. Shell scripting has its use and one of them is writing ci checks that can be understood by looking a just a few lines of code.

@antialias
Copy link
Contributor Author

Why? Banning shell scripts implies not using scripts in package.json at all - they are all run as shell scripts by npm.

package.json scripts that execute Node scripts are OS independent

The scripts are executed by the shell from which npm run was invoked. For example, this package.json script won't work as expected in Windows because && has a different meaning in powershell than is does in bash:

"test": "npm run validate && npm run test-units",

(the above line is copied and pasted from package.json in the v1.0.0 branch so we're already broken in Powershell)

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.

2 participants