-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/check ordering js #15
Feature/check ordering js #15
Conversation
Updated readme.md to show how to run test. Updated check-csv-ordering.yaml Github workflow to run the script to check ordering.
readme.md
Outdated
@@ -16,6 +16,43 @@ The submission will be reviewed and added to the Fate SRD. This usually happens | |||
|
|||
If you are comfortable with editing files and git/Github, you may [create a pull request](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request). It will be reviewed and added to the site. | |||
|
|||
There is a Github check configured to ensure the CSV file is in the expected order. This check is implemented a small | |||
Javascript script that runs under [Nodejs](https://nodejs.org/) version 20.x. (The code ill probably run under older versions, too.) |
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.
Do we want to formally declare node 20 with an .npmrc file in the repo?
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.
Another way to do that is via the "engines" key in package.json. TBH, I thought I had done that already.
Do you have a preference for which technique to use? I don't know if there is a functional difference; I think putting it in the package.json file makes it more obvious, developers will more likely read package.json than a hidden file in the repo.
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 tested the code with node 18 and node 19, both threw an error on the usage of Array.prototype.toSorted()
, so I have added in an "engines": { "node": "20.x" }
stanza to package.json, which causes node 18 and node 19 fail the npm install (or npm ci) steps.
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 left a comment and question. Here is a request—add a .gitignore file and block node_modules/
from the repo.
After that, this is good to go!
… directory in git
Added "engines" key to package.json. Updated readme.md to document the requirement.
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.
Added script
scripts/check-csv-order.js
to do the check in a system agnostic way, so we don't have to worry about having the same versions of unix utilities installed to do the check locally.Added dev dependency on node js module
csv-parse
for the script to read and parse the CSV file so that the Publisher Name and Product title can be compared independently.Hooked that script into
package.json
as the package test script.Modified
.github/workflows/check-csv-ordering.yaml
to use nodejs and thenpm test
script for the check.Updated
readme.md
with instructions on running the check locally.Added
.gitattributes
file to automatically identify text files and to enable line ending normalization.Normalized CSV file line endings in the repository.
Let me know if you run into issues with line endings on this file.