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

Update vulnerable packages and replace deprecated mocha.opts #41

Merged

Conversation

wuweiss
Copy link
Contributor

@wuweiss wuweiss commented Apr 3, 2021

No description provided.

Comment on lines +55 to +56
"mocha": {
"ui": "tdd"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has to be added as a command line argument, here:

-- node_modules/.bin/mocha

Otherwise I don't think it's going to be applied in dependent projects.

Copy link
Contributor Author

@wuweiss wuweiss Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the hint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot about mocha.opts, sorry for the misdirection. See #41 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've come full circle. Your original code was correct (prior to bf9fb04). We just need to tell other users of the lib to use the same approach. Currently the README advertises using mocha.opts. What do you think @davidchambers ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, Aldwin. I suggest the following:

Configurable via variables (min-branch-coverage) and the mocha field of package.json.

@Avaq
Copy link
Member

Avaq commented Apr 19, 2021

@wuweiss sorry for the conflicting feedback. It seems we need two things to be completed for this PR to be acceptable:

  1. Remove bf9fb04: git reset --hard 71730b2
  2. Incorporate this suggestion: Update vulnerable packages and replace deprecated mocha.opts #41 (comment) by updating the README.

Commit, force push, and we should be good to go. Right, @davidchambers ?

@davidchambers
Copy link
Member

That sounds good to me, Aldwin. :)

If you would like, @wuweiss, Aldwin or I could make these two small changes. I know what it's like to receive feedback on a pull request once my attention is elsewhere.

@wuweiss
Copy link
Contributor Author

wuweiss commented Apr 20, 2021

Sorry I was AFC, will do the changes this evening. Thanks for your help.

@wuweiss wuweiss force-pushed the updated-vuln-dependencies branch from bf9fb04 to 8b08107 Compare April 20, 2021 21:25
@davidchambers davidchambers force-pushed the updated-vuln-dependencies branch from 0adc87d to 49c8641 Compare June 6, 2021 12:39
Copy link
Member

@davidchambers davidchambers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution, @wuweiss. :)

@davidchambers davidchambers merged commit 652d2ca into sanctuary-js:master Jun 6, 2021
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.

3 participants