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

Skip package.json files with "xo": false #151

Merged
merged 1 commit into from
Oct 10, 2016

Conversation

ekmartin
Copy link
Contributor

This makes xo continue its folder recursion upwards whenever it finds a package.json where xo is set to false - see discussion at sindresorhus/atom-linter-xo#37 (comment).

There's one issue though, since pkg-conf uses Object.assign here there's no way to differ between false and an empty object - which results in this test failing. Any ideas?

@sindresorhus
Copy link
Member

Maybe we can add a Symbol to the returned object of whether the original value is false? We could then add a method like pkgConf.isFalse(returnedObject) to check it, similar to the pkgConf.filepath() method.

@sindresorhus
Copy link
Member

sindresorhus commented Oct 10, 2016

Actually, would be better to add an option in pkgConf to indicate that it should continue when it's false. That way we can contain all the logic in pkgConf instead of here. Could be useful for other tools using pkgConf too.

Suggested option name: skipOnFalse which should be false by default.

What do you think?

@sindresorhus sindresorhus changed the title Skip package.jsons with "xo": false Skip package.json files with "xo": false Oct 10, 2016
@ekmartin
Copy link
Contributor Author

ekmartin commented Oct 10, 2016

You're sure that's not slightly too opinionated? If you don't think so I can move the PR there instead.

Another option would be to only assign defaults in pkg-conf if !pkg.hasOwnProperty(namespace), and just return the actual value otherwise (i.e. here).

EDIT: Or, since you actually want the defaults to be set on objects as well, maybe check typeof pkg[namespace] === 'object' && pkg[namespace] !== null.

@sindresorhus
Copy link
Member

sindresorhus commented Oct 10, 2016

It is too opinionated indeed. That's why it should be an option that is false by default. I considered what you're suggesting, but I want to optimize for the common case and always return an object, for convenience.

A pull request on pkg-conf would be lovely :)

@ekmartin
Copy link
Contributor Author

ekmartin commented Oct 10, 2016

sindresorhus/package-config#3

By the way, could this cause issues with https://github.com/sindresorhus/xo/blob/master/cli.js#L72? Since you could potentially get the wrong package.json from meow there?

@sindresorhus
Copy link
Member

pkg-conf@2 is out, can you update this PR to use it? (Keep the tests. Those are useful as integration tests)

By the way, could this cause issues with https://github.com/sindresorhus/xo/blob/master/cli.js#L72? Since you could potentially get the wrong package.json from meow there?

No. Meow gets the package.json from find-up with cwd as __dirname, not from pkg-conf.

@sindresorhus
Copy link
Member

Could you also document the ability to pass "xo": false to skip the package.json and continue searching upwards?

@ekmartin
Copy link
Contributor Author

Done. I wasn't sure where to put the README section, so let me know if you'd rather have it somewhere else. In regards to the meow thing, wouldn't that mean that meow.pkg would become the package with "xo": false? Or am I missing something here.

Also, out of curiosity - couldn't you have released pkg-conf version 1.2.0? This doesn't seem to be a breaking change to me. Doesn't matter now though.

@sindresorhus sindresorhus merged commit fb76f88 into xojs:master Oct 10, 2016
@ekmartin ekmartin deleted the ignore_xo branch October 10, 2016 19:47
@sindresorhus
Copy link
Member

Awesome. Looks great to me. I slightly tweaked the readme change on merge. Thanks for working on this :)

Need to go to bed now, but I plan on doing a new XO release tomorrow.

In regards to the meow thing, wouldn't that mean that meow.pkg would become the package with "xo": false? Or am I missing something here.

Meow is fetching the package.json here https://github.com/sindresorhus/xo/blob/master/package.json, not from process.cwd(), and it's not even using pkg-conf.

Also, out of curiosity - couldn't you have released pkg-conf version 1.2.0? This doesn't seem to be a breaking change to me. Doesn't matter now though.

Yes, but was already planning to do a major release with sindresorhus/package-config@d31b445 to reduce the amount of dependencies.

@ekmartin
Copy link
Contributor Author

Great, thanks!

Meow is fetching the package.json here https://github.com/sindresorhus/xo/blob/master/package.json, not from process.cwd(), and it's not even using pkg-conf.

I see, makes sense.

Yes, but was already planning to do a major release with sindresorhus/package-config@d31b445 to reduce the amount of dependencies.

Ah, didn't see that you hadn't released that yet - makes perfect sense then!

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.

2 participants