-
Notifications
You must be signed in to change notification settings - Fork 3
Add uphold-scripts to allow removal of critical vulnerabilities #100
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 uphold-scripts to allow removal of critical vulnerabilities #100
Conversation
8ec3d3c to
23c9296
Compare
README.md
Outdated
|
|
||
| ```sh | ||
| ❯ npm version [<new version> | major | minor | patch] -m "Release %s" | ||
| ❯ yarn version |
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.
| ❯ yarn version | |
| ❯ yarn version [<new version> | major | minor | patch] |
23c9296 to
397d1a0
Compare
|
Travis build failing because the latest eslint requires node version >= 8. We need to change the versions under test in CI to match this. Maybe the LTS ones (8, 10, 12)? |
README.md
Outdated
|
|
||
| ```sh | ||
| ❯ npm version [<new version> | major | minor | patch] -m "Release %s" | ||
| ❯ yarn version [<new version> | major | minor | patch] |
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.
Shouldn't this be yarn release instead of yarn version?
Also, I think this would be clearer / more explicit:
❯ yarn release ["major" | "minor" | "patch" | <custom version number>] # default: patch
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.
You're right, it's release, not version, didn't even notice that.
About the rest of the text, I don't have a strong opinion, I was just recommending the generic text we're using on other projects.
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 know. 🙂 I just wanted to take the opportunity to start migrating towards a clearer phrasing (the usual one was confusing to me the first time I saw it).
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.
So, are we aiming for this one as standard?
❯ yarn release ["major" | "minor" | "patch" | <custom version number>] # default: patch
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'd at least remove the quotes there, since they might suggest the command is to be run with them.
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.
Using quotes has no disadvantage IIUC (the command works as expected), and explicitly indicates that these are specific values to be inputted as-is. So I see no downsides and a slight upside to keeping the quotes.
That said, it's not a strong preference, so if it bothers you a lot, we can remove them.
397d1a0 to
051d3b7
Compare
Makes sense, will update to those ones @rplopes. |
051d3b7 to
92d8df3
Compare
rplopes
left a comment
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.
LGTM
Description
Add
uphold-scriptswhich will allow to remove critical vulnerabilities and makes the code easier to maintain.