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

API don't respect `extend` #82

Closed
evilebottnawi opened this issue May 10, 2018 · 10 comments · Fixed by #123
Closed

API don't respect `extend` #82

evilebottnawi opened this issue May 10, 2018 · 10 comments · Fixed by #123
Assignees
Labels
Projects
Milestone

Comments

@evilebottnawi
Copy link
Contributor

@evilebottnawi evilebottnawi commented May 10, 2018

When reporting a bug, please include:

  • A little info about your environment
    • npm-package-json-lint version 3.0.0
    • npm version 6.0.0
    • node version 10.0.0
  • The output from npm-package-json-lint
    Output error only which in configuration list without rules in extend.
  • What you expected to happen
    Respect extends in api
  • The steps to reproduce the problem
    new NpmPackageJsonLint().lint(
      packageJsonData, 
      ThisVariableAcceptOnlyArrayOfRulesButShouldAcceptConfigAndRespectExtends
    );

In 2 version all works good and as expected. Thanks!

@tclindner tclindner self-assigned this May 11, 2018
@tclindner

This comment has been minimized.

Copy link
Owner

@tclindner tclindner commented May 11, 2018

Hey @evilebottnawi! v3's Node API got quite an overhaul. Calling the lint function of NpmPackageJsonLint strictly runs the linter. All of the configuration loading and processing logic has been encapsulated in the new CLIEngine. Can you please give the following a shot and see if it meets your needs? If it does, I'll try to improve the migration guide to do a better job calling out this change.

const CLIEngine = require('npm-package-json-lint').CLIEngine;

const cliEngineOptions = {
  configFile: './path/to/optional/configFile/with/extends',
  cwd: process.cwd(),
  useConfigFiles: true, // true if you want the CLIEngine to automatically look for .npmpackagejsonlintrc.json and npmpackagejsonlint.config.js files in the project hierarchy 
  rules: {} // rules object
};

const cliEngine = new CLIEngine(cliEngineOptions);
const patterns = ['./path/to/package.json/file'];
const results = cliEngine.executeOnPackageJsonFiles(patterns);
@evilebottnawi

This comment has been minimized.

Copy link
Contributor Author

@evilebottnawi evilebottnawi commented May 11, 2018

@tclindner when i use linter programmly, using CLIEngine looks is misleading 😕

@tclindner

This comment has been minimized.

Copy link
Owner

@tclindner tclindner commented May 12, 2018

Hi @evilebottnawi - I'm sorry that you find the new API is misleading. 😢 If you prefer to use the linter directly you still can. Does something like this feel better to you?

const CLIEngine = require('npm-package-json-lint').CLIEngine;
const NpmPackageJsonLint = require('npm-package-json-lint').NpmPackageJsonLint;

const cliEngine = new CLIEngine({configFile: './path/to/optional/configFile/with/extends'});
const config = cliEngine.getConfigForFile('./path/to/package.json');

new NpmPackageJsonLint().lint(
  packageJsonData, 
  config.rules
);
@evilebottnawi

This comment has been minimized.

Copy link
Contributor Author

@evilebottnawi evilebottnawi commented May 12, 2018

@tclindner i already found solution, like you write above. I think we can fix misleading api in version 4. Just allow linting programmatically without creating CLIEngine (stylelint/eslint allow this) 👍

@tclindner

This comment has been minimized.

Copy link
Owner

@tclindner tclindner commented May 14, 2018

Hey @evilebottnawi and @ntwb - I took a look at ESLint's Node.js API and it looks like their linter class takes in a config object too. They note that:

a configuration object that has been processed and normalized by CLIEngine using eslintrc files and/or other configuration arguments.

How are you using their linter without needing the CLIEngine to process the configuration?

@tclindner tclindner added this to To do in v4 via automation Jun 16, 2019
@tclindner tclindner added this to the v4.0.0 milestone Jun 16, 2019
@tclindner

This comment has been minimized.

Copy link
Owner

@tclindner tclindner commented Jun 22, 2019

Hey @evilebottnawi what do you think of the previous comment? It seems like ESLint follows this same practice. I'm happy to make changes if you can help me understand the differences between how you use ESLint's API and npm-package-json-lint's API.

@ntwb

This comment has been minimized.

Copy link

@ntwb ntwb commented Jul 5, 2019

I don't think ESLint uses cosmic config, but we do in stylelint

https://www.npmjs.com/package/cosmiconfig

Is that what you were thinking of here @evilebottnawi ?

(@evilebottnawi and I are both on the @stylelint team)

@evilebottnawi

This comment has been minimized.

Copy link
Contributor Author

@evilebottnawi evilebottnawi commented Jul 5, 2019

i think using cosmiconfig is good idea, we need this API for test our configurations, we solve this on our side, but will be great if problem will be solved on package level

@tclindner

This comment has been minimized.

Copy link
Owner

@tclindner tclindner commented Jul 8, 2019

Thanks, @evilebottnawi and @ntwb. I've started implementing cosmicconfig based on your recommendation. I don't think that has any impact on the public API of this module, though. Can you confirm that you would like is a single method, lint, that applies all config lookup/extends/etc. when invoked?

@evilebottnawi

This comment has been minimized.

Copy link
Contributor Author

@evilebottnawi evilebottnawi commented Jul 8, 2019

Yes, linting should load extended config 👍

tclindner added a commit that referenced this issue Jul 27, 2019
Closes #82
@tclindner tclindner mentioned this issue Jul 27, 2019
1 of 1 task complete
v4 automation moved this from To do to Done Jul 27, 2019
tclindner added a commit that referenced this issue Jul 27, 2019
* Add logic to apply overrides

Closes #96

* Update eslint-config-tc

* 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

* Update cosmicConfigTransformer.js

* Ignore lint temporarily for beta

* Fix file paths

* Update ConfigValidator.test.js

* Update ConfigValidator.test.js

* Update NpmPackageJsonLint.test.js

* Update getFileList.js

* Update getFileList.js

* Add default for base config directory

* Add additional tests for overrides and local build script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v4
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.