Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Add support for two package.json structure #37

Closed
matheuss opened this issue Sep 23, 2016 · 15 comments · Fixed by #40
Closed

Add support for two package.json structure #37

matheuss opened this issue Sep 23, 2016 · 15 comments · Fixed by #40

Comments

@matheuss
Copy link
Contributor

matheuss commented Sep 23, 2016

If you're viewing files inside an app folder on a two package.json structure, you'll not see any XO reporting 😕

We just need some better logic here https://github.com/sindresorhus/atom-linter-xo/blob/master/index.js#L32

@leo
Copy link

leo commented Sep 23, 2016

We need that over at https://github.com/zeit/hyperterm

@sindresorhus
Copy link
Owner

@matheuss What do you suggest we do?

@matheuss
Copy link
Contributor Author

matheuss commented Sep 24, 2016

We could check if the parent directory has a package.json with: electron|electron-packager|electron-builder and xo. What do you think?

Also we could check if the current directory is an app.

@sindresorhus
Copy link
Owner

I guess... Only really need to check for electron-builder.

@matheuss
Copy link
Contributor Author

Ohh that's true, my bad. So checking if the current directory is named app and the parent package.json has electron-builder and xo sounds good to you?

@sindresorhus
Copy link
Owner

Yes, except the app part. Not everyone uses the app directory name and it wouldn't work if you were in app/foo/.

@ekmartin
Copy link

ekmartin commented Oct 7, 2016

Does it have to have anything to do with electron? Couldn't it just recurse upwards until it found a package.json with XO in it? Maybe with a few safety checks, like stopping whenever it finds a package.json with any eslint dependencies in them at all?

@sindresorhus
Copy link
Owner

@ekmartin That sounds nice in theory, but would be surprising when it suddenly starts using a XO config far up in the directory tree because none other contained XO.

Another more generic solution would be to support skipping a package.json by just defining "xo": false in package.json, but I didn't mention it as there has never been any use-case for this outside of electron-builder.

@ekmartin
Copy link

ekmartin commented Oct 7, 2016

That would work too. I'm struggling to see a situation where users would have deeply nested package.json folders without wanting to impose some kind of structure though.

A few scenarios I see are:

  • You have nested projects where you want to use the parent's config for linting (like Hyper).
  • You have nested projects where you want to use the parent's config for a few of the folders, but not all. This should still work since atom-linter-xo would respect the ignore options of the parent, right?
  • You have nested project folders where you use different linters for different subprojects. This is the most fragile one, but I think if you were to check for something that resembles a linter in the closest package.json you could work around it in a decent way.

What do you think? If not maybe something like "xo": { "useParent": true } could be good, like you're suggesting.

@sindresorhus
Copy link
Owner

You might but right, but who knows. You'd be surprised how many weird and unexpected things people do. I've seen people accidentally put a config file that is meant to be local in their home directory and getting all kinds of weird hard to debug problems because it was walking all the way up to the home directory.

I was kinda hoping to just make it work for this actual use-case, instead of trying to come up with a more generic solution for other imaginary use-cases.

@ekmartin
Copy link

ekmartin commented Oct 7, 2016

Yeah, that makes sense as well. Up to you!

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 10, 2016

Ok, I've decided. I think "xo": false would be the best way. Having to recurse up to check for electron-builder has the same problems as described above, that it can possibly recurse all the way up to the home directory. This would also be a slight perf decrease for everyone, just for the benefit of a few electron-builder users. I also like "xo": false because it's explicit. No magic!

PR welcome on the main XO project :)

@ekmartin
Copy link

@sindresorhus: xojs/xo#151

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 11, 2016

New XO version is out: https://github.com/sindresorhus/xo/releases/tag/v0.17.0
You also need to upgrade this plugin.

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 11, 2016

Ugh, I was wrong. This it not totally done. I forgot that this plugin reads the package.json too. That logic should really be moved to XO itself, but I'd be ok adding a quick workaround for now. Can just check if the package.json contains "xo": false and if so try pkgDir.sync sync again with the parent dir as cwd.

A pull request would be appreciated, as I don't think I'll get to this until next week.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants