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

Don't search node_modules if . is listed as a source dependency. #278

Closed
jackfranklin opened this issue Nov 18, 2016 · 11 comments
Closed

Don't search node_modules if . is listed as a source dependency. #278

jackfranklin opened this issue Nov 18, 2016 · 11 comments
Milestone

Comments

@jackfranklin
Copy link

Taken from avh4/elm-upgrade#17 (comment).

@eeue56
Copy link
Contributor

eeue56 commented Dec 7, 2016

node_modules should not be in your Elm source-directories.

@jackfranklin
Copy link
Author

It wasn't. I had a node_module installed that had some example/Foo.elm that elm-upgrade picked up. Maybe it's not limiting its search correctly?

@eeue56
Copy link
Contributor

eeue56 commented Dec 7, 2016

Can you explain what your directory tree looks like? Can you explain what you mean by source dependency?

@jackfranklin
Copy link
Author

I have Elm files in the root directory, and my elm-package.json listed the src directories as just ..

I have a node dependency in node_modules/gulp-elm that includes a dummy Elm file at: node_modules/gulp-elm/examples/Demo.elm.

When I ran elm-upgrade (which in turn ran elm-format) it tried to format node_modules/gulp-elm/examples/Demo.elm, which I wouldn't expect.

@rundis
Copy link
Contributor

rundis commented Dec 7, 2016

Don't use . (dot aka everything) as a source directory in elm-package.json ?

@jackfranklin
Copy link
Author

Sure, this is an older project and I have moved onto src by default.

I do think a lot of people use . though and I would argue that ignoring node_modules by default is sensible (or perhaps ignoring anything that's ignored by Git?).

That said it might not be as big of a deal, in which case feel free to close this.

@eeue56
Copy link
Contributor

eeue56 commented Dec 7, 2016

Right, yeah. node_modules should not be in your source-directories, really. If it's in the same dir as elm-package.json, and you list . as a directory, then it's going to look in there. And it should look in there.

I think adding an --exclude flag mgiht be helpful.

@eeue56
Copy link
Contributor

eeue56 commented Dec 7, 2016

Ignoring things that are ignored in .gitignore is generally not a good way to do things. It introduces magic into the system, and reduces the amount you can rely on it.

@jackfranklin
Copy link
Author

Fair enough, I can buy that arguments. I do think having node_modules as a default excluded/blacklisted folder would be sensible but I appreciate not wanting magic.

@avh4
Copy link
Owner

avh4 commented Dec 7, 2016

I think the original issue here was fine. It's common to have . as a source directory, and it's common to have ./node_modules. It's also common to have ./tests/node_modules when . is a source directory.

I'm leaving this issue as described.

Please open a new discussion issue if you want to debate alternative solutions.

@avh4
Copy link
Owner

avh4 commented Mar 21, 2017

Fixed in 021b55a

@avh4 avh4 added this to the 0.6.0 public beta milestone Mar 21, 2017
@avh4 avh4 closed this as completed Mar 21, 2017
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

No branches or pull requests

4 participants