Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
invert npmignore to include what we want #4605
invert npmignore to include what we want #4605
Changes from 6 commits
dc94e43
801d364
0329067
7ba622c
accce64
3003f2e
fb2d91c
938376b
e0e095d
6580244
3afab2d
39cf0bf
fb349db
e1502ac
16bcdfa
f772a99
c5a60f9
ef8f61d
2944845
8c94f0f
c1a547b
2bb4a07
fe0a0a8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdyt think about this TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/src/utils/testing.js
was previously being published to npm.I added a
/src/**/test/
rule ignore anything intest/
folders. It would make sense to put all test files (apart from the specs files that may be co-located with the thing they test) intest/
folders, to make it easier to ignore them without having to modifynpmignore
.We can add one-off rules for specific test files to ignore as we go, but I think it would be better to have the
test/
folder convention, otherwise it's the same problem this PR aimed to eliminate (but in reverse).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trusktr maybe we could file an issue for that chance and include a link to that issue in this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a naming convention of Spec. why can't we use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shefalijoshi Yes, true that, but multiple spec files like to import test-specific utilities from
src/utils/testing.js
so I had to specifically ignore that file because it isn't following a catch-all convention. So what I meant above is that instead we could move that file tosrc/test/utils.js
instead, and that way it will fall under thesrc/**/test/*
catch-all instead of having a one-off entry in npmignore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do go with this approach, we should add
example
to this too. It haseventGenerator
andgenerator
which a lot of people use to demo OpenMCTThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trusktr Please include the example, docs folders and Contributing.md, API.md, app.js as well.
What is the procfile used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to publish all those things in the package on NPM? I figured people can go to the repo for that stuff. People don't normally look for that stuff in
node_modules
. EDIT: Oh I see @scottbell, so some people will want to import from examples.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do people consume
app.js
fromnode_modules
? My understanding is we want to obliterate app.js ( @akhenry) and I think it is a good idea.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.google.com/search?q=procfile&oq=procfile&aqs=edge..69i57.870j0j1&sourceid=chrome&ie=UTF-8
Not sure why we need that. Are we using it on some service? If some downstream project needs it, they should have that in their repo instead.