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 .nvmrc #546

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
3 participants
@ashishmohite
Copy link
Contributor

ashishmohite commented Oct 1, 2018

  • improves build reproducibility by switching to an appropriate node version
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 1, 2018

Codecov Report

Merging #546 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #546   +/-   ##
=======================================
  Coverage   74.56%   74.56%           
=======================================
  Files          97       97           
  Lines        6391     6391           
=======================================
  Hits         4765     4765           
  Misses       1284     1284           
  Partials      342      342

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0c3039...ddac26f. Read the comment docs.

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 1, 2018

Thanks for your contribution. I like the idea of having a .nvmrc as some use it a lot, although I’m not a big fan of adding it as a dependency to the contributing. The less tool we require, the less friction it creates for new contributors to join.

@goenning goenning self-requested a review Oct 1, 2018

@ashishmohite

This comment has been minimized.

Copy link
Contributor

ashishmohite commented Oct 2, 2018

@goenning can I add "(optional)" in front of the requirement for nvm ? or should just remove it from the requirements ?

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 2, 2018

I’d prefer to revert that file and add a note beside the Node.js requirement, something like:

What you think?

@ashishmohite ashishmohite force-pushed the ashishmohite:add_nvmrc branch from 8dd38da to 3623eed Oct 2, 2018

@ashishmohite

This comment has been minimized.

Copy link
Contributor

ashishmohite commented Oct 2, 2018

Done

@goenning goenning merged commit ed01b76 into getfider:master Oct 2, 2018

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: push Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test-server Your tests passed on CircleCI!
Details
ci/circleci: test-ui Your tests passed on CircleCI!
Details
@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 2, 2018

@ashishmohite merged. Thanks! 😀

@ashishmohite ashishmohite deleted the ashishmohite:add_nvmrc branch Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment