invert npmignore to include what we want #4605

trusktr · 2021-12-17T07:46:52Z

Describe your changes:

instead of excluding a set of files that is more likely to change over time (f.e. ops files, build scripts, ci configs, etc)

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Unit tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue?

To test, use npm pack to create a tar file, then verify the contents.

Reviewer Checklist

Changes appear to address issue?
Changes appear not to be breaking changes?
Appropriate unit tests included?
Code style and in-line documentation are appropriate?
Commit messages meet standards?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

codecov · 2021-12-17T07:51:13Z

Codecov Report

Merging #4605 (2bb4a07) into master (9643dbe) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 2bb4a07 differs from pull request most recent head fe0a0a8. Consider uploading reports for the commit fe0a0a8 to get more accurate results

@@            Coverage Diff             @@
##           master    #4605      +/-   ##
==========================================
- Coverage   50.16%   50.15%   -0.02%     
==========================================
  Files         513      513              
  Lines       18830    18830              
  Branches     1664     1664              
==========================================
- Hits         9447     9444       -3     
- Misses       8965     8969       +4     
+ Partials      418      417       -1

Impacted Files	Coverage Δ
src/api/objects/ObjectAPI.js	`91.16% <0.00%> (-2.21%)`	⬇️
src/ui/inspector/details/Properties.vue	`60.71% <0.00%> (-1.79%)`	⬇️
src/plugins/imagery/components/ImageryTimeView.vue	`71.83% <0.00%> (-0.58%)`	⬇️
...c/plugins/persistence/couch/CouchObjectProvider.js	`82.22% <0.00%> (+0.44%)`	⬆️
src/plugins/timeConductor/ConductorHistory.vue	`51.06% <0.00%> (+2.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dad0768...fe0a0a8. Read the comment docs.

trusktr · 2021-12-17T07:52:44Z

How is it that this increases test coverage?

unlikelyzero · 2021-12-17T14:59:21Z

@trusktr looks good to me. Let's wait until we get the stray .spec.js file removed, update this pr, and then merge. PRs have a habit of getting merged out of order sometimes.

unlikelyzero · 2021-12-17T14:59:47Z

@shefalijoshi please take a look at this with @trusktr . This pattern seems to be far more sustainable.

trusktr · 2021-12-17T19:36:45Z

I believe this PR can be merged separately from fixing that test. The pull request that fixes that test would have to remove the line with .spec.js from the npmignore.

unlikelyzero

LGTM. Needs Shefali's approval and the two lines removed

.npmignore

trusktr · 2022-01-04T19:19:25Z

@shefalijoshi @unlikelyzero I removed the no-longer-needed line, and also added a # TODO

trusktr · 2022-01-04T19:20:47Z

.npmignore

+/src/**/*Spec.js
+/src/**/test/
+# TODO move test utils into test/ folders
+/src/utils/testing.js


Wdyt think about this TODO?

/src/utils/testing.js was previously being published to npm.

I added a /src/**/test/ rule ignore anything in test/ 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) in test/ folders, to make it easier to ignore them without having to modify npmignore.

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).

@trusktr maybe we could file an issue for that chance and include a link to that issue in this comment?

We have a naming convention of Spec. why can't we use that?

@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 to src/test/utils.js instead, and that way it will fall under the src/**/test/* catch-all instead of having a one-off entry in npmignore.

If we do go with this approach, we should add example to this too. It has eventGenerator and generator which a lot of people use to demo OpenMCT

If we do go with this approach, we should add example to this too. It has eventGenerator and generator which a lot of people use to demo OpenMCT

@trusktr Please include the example, docs folders and Contributing.md, API.md, app.js as well.
What is the procfile used for?

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.

Do people consume app.js from node_modules? My understanding is we want to obliterate app.js ( @akhenry) and I think it is a good idea.

What is the procfile used for?

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.

trusktr · 2022-01-04T19:24:05Z

To test, use npm pack to create a tar file, then verify the contents.

unlikelyzero

LGTM. I had one suggested change that won't block the merge. Shefali is the release manager so she should give the final 👍

unlikelyzero · 2022-01-04T19:34:55Z

.npmignore

+/src/**/*Spec.js
+/src/**/test/
+# TODO move test utils into test/ folders
+/src/utils/testing.js


@trusktr maybe we could file an issue for that chance and include a link to that issue in this comment?

trusktr · 2022-03-22T21:19:34Z

maybe we could file an issue for that chance and include a link to that issue in this comment?

@unlikelyzero made a new issue #4972

trusktr · 2022-03-22T21:37:01Z

@shefalijoshi wrote:

Please include the example, docs folders and Contributing.md, API.md, app.js as well.

Do we need to includes all those? Looking for thoughts. Will put a checkbox next to the ones to include

app.js: Andrew expressed we should remove, and I agree
examples folder: do people consume stuff by importing from node_modules/openmct/examples? If not, then they can run examples from the git repo after cloning it
docs folder: people can see that in the repo, and on the website, so it doesn't seem we need to ship it to the package registry
contributing.md: also visible in the repo, where contributors will be going to contribute anyway.

unlikelyzero · 2022-03-22T23:11:14Z

Do we need to includes all those? Looking for thoughts. Will put a checkbox next to the ones to include

app.js: Andrew expressed we should remove, and I agree
examples folder: do people consume stuff by importing from node_modules/openmct/examples? If not, then they can run examples from the git repo after cloning it
docs folder: people can see that in the repo, and on the website, so it doesn't seem we need to ship it to the package registry
contributing.md: also visible in the repo, where contributors will be going to contribute anyway.

App.js has a removal ticket here: #4922 if something needs to be removed, we could also remove at that time.

shefalijoshi

Looks good to me.

.npmignore

scottbell · 2022-03-23T12:31:16Z

* [ ]  examples folder: do people consume stuff by importing from `node_modules/openmct/examples`? If not, then they can run examples from the git repo after cloning it

I'm not sure as I don't have data to say one way or another, but the Sin Wave, State, and Event generator seem to get a lot of use in openmct deployments.

…o dont-publish-test-files

jvigliotta

LGTM!

invert npmignore to include what we want

dc94e43

trusktr requested a review from unlikelyzero December 17, 2021 07:46

trusktr mentioned this pull request Dec 17, 2021

Improve npmignore #4602

Closed

trusktr assigned trusktr and unlikelyzero Dec 17, 2021

unlikelyzero requested a review from shefalijoshi December 17, 2021 14:58

unlikelyzero assigned shefalijoshi Dec 17, 2021

Merge branch 'master' into dont-publish-test-files

801d364

unlikelyzero mentioned this pull request Dec 20, 2021

[CI] Determine why codecov.io is reporting ~50% coverage instead of ~70% #4563

Closed

unlikelyzero requested changes Dec 30, 2021

View reviewed changes

.npmignore Outdated Show resolved Hide resolved

unlikelyzero and others added 4 commits December 29, 2021 16:27

Merge branch 'master' into dont-publish-test-files

0329067

Merge branch 'master' into dont-publish-test-files

7ba622c

Merge branch 'master' into dont-publish-test-files

accce64

remove line for no-longer-existent src/**/*.spec.js files from npmignore

3003f2e

trusktr commented Jan 4, 2022

View reviewed changes

trusktr requested review from unlikelyzero and shefalijoshi and removed request for shefalijoshi January 4, 2022 19:23

unlikelyzero previously approved these changes Jan 4, 2022

View reviewed changes

trusktr and others added 3 commits January 4, 2022 13:59

Merge branch 'master' into dont-publish-test-files

fb2d91c

Merge branch 'master' into dont-publish-test-files

938376b

cleanup npmignore comments

e0e095d

trusktr dismissed unlikelyzero’s stale review via e0e095d January 5, 2022 01:36

remove platform dirs from npmignore, they were removed from the repo

16bcdfa

trusktr requested review from shefalijoshi, jvigliotta and unlikelyzero March 22, 2022 21:20

shefalijoshi requested a review from scottbell March 22, 2022 23:24

shefalijoshi previously approved these changes Mar 22, 2022

View reviewed changes

Merge branch 'master' into dont-publish-test-files

f772a99

shefalijoshi enabled auto-merge (squash) March 22, 2022 23:25

jvigliotta requested changes Mar 22, 2022

View reviewed changes

.npmignore Outdated Show resolved Hide resolved

remove one more platform reference from npmignore

c5a60f9

trusktr dismissed shefalijoshi’s stale review via c5a60f9 March 23, 2022 00:54

Merge branch 'master' into dont-publish-test-files

ef8f61d

shefalijoshi disabled auto-merge March 23, 2022 15:46

trusktr added 4 commits March 23, 2022 10:09

Merge branch 'master' into dont-publish-test-files

2944845

publish example/ and app.js, at least for now

8c94f0f

Merge branch 'dont-publish-test-files' of github.com:nasa/openmct int…

c1a547b

…o dont-publish-test-files

Merge branch 'master' into dont-publish-test-files

2bb4a07

unlikelyzero requested a review from jvigliotta March 23, 2022 22:29

unlikelyzero approved these changes Mar 23, 2022

View reviewed changes

jvigliotta enabled auto-merge (squash) March 23, 2022 22:33

Merge branch 'master' into dont-publish-test-files

fe0a0a8

jvigliotta requested review from jvigliotta and removed request for jvigliotta March 23, 2022 22:34

jvigliotta approved these changes Mar 23, 2022

View reviewed changes

jvigliotta merged commit 0705d32 into master Mar 23, 2022

trusktr deleted the dont-publish-test-files branch March 28, 2022 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

invert npmignore to include what we want #4605

invert npmignore to include what we want #4605

trusktr commented Dec 17, 2021 •

edited by shefalijoshi

Loading

codecov bot commented Dec 17, 2021 •

edited

Loading

trusktr commented Dec 17, 2021

unlikelyzero commented Dec 17, 2021

unlikelyzero commented Dec 17, 2021

trusktr commented Dec 17, 2021

unlikelyzero left a comment

trusktr commented Jan 4, 2022 •

edited

Loading

trusktr Jan 4, 2022

trusktr Jan 4, 2022 •

edited

Loading

unlikelyzero Jan 4, 2022

shefalijoshi Jan 4, 2022

trusktr Jan 4, 2022

scottbell Jan 13, 2022

shefalijoshi Jan 20, 2022

trusktr Jan 31, 2022 •

edited

Loading

trusktr Jan 31, 2022 •

edited

Loading

trusktr Mar 22, 2022

trusktr commented Jan 4, 2022

unlikelyzero left a comment

unlikelyzero Jan 4, 2022

trusktr commented Mar 22, 2022

trusktr commented Mar 22, 2022 •

edited

Loading

unlikelyzero commented Mar 22, 2022 •

edited

Loading

shefalijoshi left a comment

scottbell commented Mar 23, 2022 •

edited

Loading

jvigliotta left a comment

invert npmignore to include what we want #4605

invert npmignore to include what we want #4605

Conversation

trusktr commented Dec 17, 2021 • edited by shefalijoshi Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

codecov bot commented Dec 17, 2021 • edited Loading

Codecov Report

trusktr commented Dec 17, 2021

unlikelyzero commented Dec 17, 2021

unlikelyzero commented Dec 17, 2021

trusktr commented Dec 17, 2021

unlikelyzero left a comment

Choose a reason for hiding this comment

trusktr commented Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

trusktr Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trusktr Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

trusktr Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trusktr commented Jan 4, 2022

unlikelyzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trusktr commented Mar 22, 2022

trusktr commented Mar 22, 2022 • edited Loading

unlikelyzero commented Mar 22, 2022 • edited Loading

shefalijoshi left a comment

Choose a reason for hiding this comment

scottbell commented Mar 23, 2022 • edited Loading

jvigliotta left a comment

Choose a reason for hiding this comment

trusktr commented Dec 17, 2021 •

edited by shefalijoshi

Loading

codecov bot commented Dec 17, 2021 •

edited

Loading

trusktr commented Jan 4, 2022 •

edited

Loading

trusktr Jan 4, 2022 •

edited

Loading

trusktr Jan 31, 2022 •

edited

Loading

trusktr Jan 31, 2022 •

edited

Loading

trusktr commented Mar 22, 2022 •

edited

Loading

unlikelyzero commented Mar 22, 2022 •

edited

Loading

scottbell commented Mar 23, 2022 •

edited

Loading