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

Fix check for root node in the event it does not exist #84

Merged
merged 3 commits into from
May 15, 2018
Merged

Conversation

tclindner
Copy link
Owner

@tclindner tclindner commented May 15, 2018

Fixes #79

** Description of change **
Resolves an issue reported by @ntwb in #79. The conditional logic that checks the value of the root property flag didn't properly account for files that didn't have a root property set.

** Checklist **

  • Unit tests have been added
  • Specific notes for documentation, if applicable

@ntwb
Copy link
Contributor

ntwb commented May 15, 2018

I just had a quick look at this PR, and just to make sure we are both on the same page, the fixtures you created hierarchyWithoutRoot...

This is so that sub-directory package.json files are linted but not the root package.json file though the configuration is read from the root package.json file?

@ntwb
Copy link
Contributor

ntwb commented May 15, 2018

I just checked out the 7920c02 commit and it works as expected without error now 👍

i.e. ./node_modules/.bin/npmPkgJsonLint packages/ worked perfectly 👌

(I'd also deleted my .npmpackagejsonlintrc.json file to test that the config was read from the root package.json file )

@tclindner
Copy link
Owner Author

Hey @ntwb! Great question. Let me know if this explanation doesn't make sense.

Scenario 1: A package.json exists in the root of the repo and a package.json exists in a subdirectory, subdir. Both package.json files have npmPackageJsonLintConfig properties. Consider you want to lint the subdir package.json file and use rules from its property and the root package.json file property. This can be done by not adding a root property to the subdir package.json file or adding it but setting it to false.

Scenario 2: A package.json exists in the root of the repo and a package.json exists in a subdirectory, subdir. Only the root package.json file has a npmPackageJsonLintConfig property. The config logic will now automatically look at the parent for config.

Scenario 3: A package.json exists in the root of the repo and a package.json exists in a subdirectory, subdir. Both package.json files have npmPackageJsonLintConfig properties. Consider you want to want to lint the subdir package.json file and only use rules from its property. This can be done by adding a root property to the subdir package.json file and setting it to true.

Scenario 4: A package.json exists in the root of the repo and a package.json exists in a subdirectory, subdir. Only the root package.json files has a npmPackageJsonLintConfig property. Consider you only want to lint the subdir package.json file and use rules from its root package.json file property. This can be done using the cli to glob for only the subdir package.json file. The config will automatically get picked up.

@tclindner tclindner merged commit 10348dd into master May 15, 2018
@tclindner tclindner deleted the fix-79 branch May 15, 2018 02:20
@tclindner
Copy link
Owner Author

Awesome! I'm so happy it worked for you. I just published v3.0.1. Thank you so much for your help identifying and resolving this issue! 🥇

@ntwb
Copy link
Contributor

ntwb commented May 15, 2018

Thanks @tclindner, and the above makes perfect sense and I've already got a use case in mind for nested configurations 👍

@tclindner
Copy link
Owner Author

Cool! I would love to hear about what you implement. We could use them as examples in the docs, if you want. 🤓

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