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

Allow .json extension for rc file #38

Closed
wants to merge 1 commit into from
Closed

Allow .json extension for rc file #38

wants to merge 1 commit into from

Conversation

DecentM
Copy link

@DecentM DecentM commented Apr 21, 2018

Many code editors and IDEs have syntax highlighting and linting based on the file extension. This PR adds a check to see if an .env-cmdrc.json exists. If so, it will use that instead.

Todo

  • Write solution
  • Add notes in documentation
  • Write tests

Notable changes

  • Add check to see if .env.cmdrc.json exists - env-cmd will use that first
  • Add notes in README.md

Before merging

I haven't written tests for the new check, because starting with line DecentM/env-cmd/test/test.js#208, the whole of existsSync and readFileSync are stubbed. It looks like 100% coverage would require a different way of mocking. Can you point me in the right direction to solve this?

* Add check to see if .env.cmdrc.json exists - `env-cmd` will use that first
* Add notes in README.md
Copy link
Owner

@toddbluhm toddbluhm left a comment

Choose a reason for hiding this comment

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

This is a good start. What I would suggest is instead of executing that .forEach inline, I would create a new function called GetRCFileLocation and then do rcFileLocation = GetRCFileLocation().

Now you have isolated that new business logic into a separate testable function. Export the function and create a new describe section in the test cases. Then stub out the existsSync call as I have previously done, but this time instead of just returning true, you can use sinons onCall() method to determine what to return for each call. Play around with that and counting the number of time execSync is called as well as checking the return results of GetRCFileLocation.

Combining all of that should provide sufficient coverage. You will probably end up with 3 tests out of that.
1 - non-json rc return
2 - .json rc return
3 - files doesn't exist at all return (null)

Hopefully all of that makes sense.

rcFilenames.forEach((filename) => {
const rcPath = path.join(process.cwd(), filename)

if (fs.existsSync(rcPath)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I believe we should check for the existence of rcFileLocation here as well. That way we don't overwrite the previously set version.

@toddbluhm
Copy link
Owner

This has been implemented in the new 9.0.0 release 🎉

@toddbluhm toddbluhm closed this May 4, 2019
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.

None yet

2 participants