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

Config overrides api #123

Merged
merged 27 commits into from Jul 27, 2019
Merged

Config overrides api #123

merged 27 commits into from Jul 27, 2019

Conversation

tclindner
Copy link
Owner

Description of change
Add logic to apply overrides - Closes #96
Add cosmicconfig and switch to globby
Add new utils for ignore and file list
Add error handling to config
Add typedef to LintIssue
Add new linter and results helper
Add tests for utils
Tests for new linter
Add test for absolute paths
Add tests for overrides and extends
Add ignore support to cli reporter
Update api now that CLIEngine is no longer exported
Update Reporter.js
Add initial version of transformer
Add config tests
Update CHANGELOG.md
Closes #82

Checklist

  • Unit tests have been added

@tclindner tclindner merged commit 8f93878 into master Jul 27, 2019
@tclindner tclindner deleted the config-overrides-api branch July 27, 2019 21:06
@simison
Copy link
Contributor

simison commented Oct 4, 2019

This is very cool! Would it be possible to get a new NPM release with this feature? :-)

@tclindner
Copy link
Owner Author

Definitely! Sorry for the delay 🙈

/**
* Applies values from the 'overrides' field in a configuration file.
* @param {string} cwd The current working directory.
* @param {Object} filePath The file path of the file being linted.
Copy link
Contributor

Choose a reason for hiding this comment

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

@tclindner is this working as intended for you? I'm seeing the path of the config file as filePath instead of file being linted. Therefore filePath === globbedFilePath is never truthy and overrides don't work.

Note how path gets pulled from config object here before passed to applyOverrides:

https://github.com/tclindner/npm-package-json-lint/pull/123/files#diff-4c88ce73413b52beff7db767b94bd2f4R11

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hey @simison - it seems to be working for my basic setup. What does your setup look like? We can try to get a patch in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, well you just got me to doubt myself and if I was testing thoroughly enough! 😅

Here's the config I'm working on Automattic/wp-calypso#36534

No worries if you don't have time, I can also dig deeper into this.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thank you, @simison! I think I have a fix ready. Please give v4.0.1 a shot and let me know if it resolves your issue. Thanks again for reporting it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yasss, that worked fantastically! Thank you!

tclindner added a commit that referenced this pull request Oct 19, 2019
tclindner added a commit that referenced this pull request Oct 19, 2019
* Fix an issue with how config overrides get applied

Please see #123 for discussion with @simison.

* Update getFileList.test.js
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.

Support overrides API don't respect extend
2 participants