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.

Sign up for GitHub

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

Jump to bottom

List files in a less verbose way by auto-grouping them #690

Closed
Drarig29 opened this issue Apr 9, 2023 · 6 comments · Fixed by #716
Closed

List files in a less verbose way by auto-grouping them #690

Drarig29 opened this issue Apr 9, 2023 · 6 comments · Fixed by #716

Comments

@Drarig29
Copy link
Contributor

Drarig29 commented Apr 9, 2023

Description

Having an exhaustive list of files isn't always interesting, especially when it gets too verbose.

Is the feature request related to a problem?

cf. #681 (comment)

Possible implementation

Currently, in those 2 cases all files are exhaustively listed with util.joinList():

np/source/ui.js

Lines 95 to 102 in eba203f

const messages = [];
if (newFiles.unpublished.length > 0) {
messages.push(`The following new files will not be part of your published package:\n${util.joinList(newFiles.unpublished)}`);
}
if (newFiles.firstTime.length > 0) {
messages.push(`The following new files will be published for the first time:\n${util.joinList(newFiles.firstTime)}`);
}

I think the 2nd case The following new files will be published for the first time is far more important than the 1st one because those files will end up in a package.

So IMO, only the files of the 1st case The following new files will not be part of your published package should be auto-grouped if necessary.

Alternatives

Or maybe both of the 2 cases should have auto-grouping. But the dry run would disable auto-grouping and show all the files?
@Drarig29
Copy link
Contributor Author

Drarig29 commented Apr 9, 2023

I found an implementation which turns this:

- test/_utils.js
- test/_utils-1.js
- test/_utils-2.js
- test/_utils-3.js
- test/_utils-4.js
- test/_utils-5.js
- test/_utils-6.js
- test/_utils-7.js
- test/_utils-8.js
- test/_utils-9.js
- test/_utils-10.js
- test/_utils-11.js
- test/_utils-12.js
- test/_utils-13.js
- test/fixtures/config/homedir4/.np-config.js
- test/fixtures/config/homedir5/.np-config.mjs
- test/fixtures/config/local4/.np-config.js
- test/fixtures/config/local4/package.json
- test/fixtures/config/local5/.np-config.mjs
- test/fixtures/config/package.json
- test/fixtures/files/dot-github/.github/pull_request_template.md
- test/fixtures/files/dot-github/index.js
- test/fixtures/files/dot-github/package.json
- test/fixtures/files/files-and-npmignore/package.json
- test/fixtures/files/files-and-npmignore/readme.md
- test/fixtures/files/files-and-npmignore/source/.npmignore
- test/fixtures/files/files-and-npmignore/source/bar.js
- test/fixtures/files/files-and-npmignore/source/foo.js
- test/fixtures/files/files-and-npmignore/source/index.d.ts
- test/fixtures/files/files-and-npmignore/source/index.test-d.ts
- test/fixtures/files/files-slash/index.js
- test/fixtures/files/files-slash/package.json
- test/fixtures/files/gitignore/dist/index.js
- test/fixtures/files/gitignore/gitignore
- test/fixtures/files/gitignore/index.d.ts
- test/fixtures/files/gitignore/index.js
- test/fixtures/files/gitignore/index.test-d.ts
- test/fixtures/files/gitignore/package.json
- test/fixtures/files/gitignore/readme.md
- test/fixtures/files/has-readme-and-license/index.js
- test/fixtures/files/has-readme-and-license/license.md
- test/fixtures/files/has-readme-and-license/package.json
- test/fixtures/files/has-readme-and-license/readme.md
- test/fixtures/files/main/bar.js
- test/fixtures/files/main/foo.js
- test/fixtures/files/main/package.json
- test/fixtures/files/npmignore-and-gitignore/.npmignore
- test/fixtures/files/npmignore-and-gitignore/dist/index.js
- test/fixtures/files/npmignore-and-gitignore/gitignore
- test/fixtures/files/npmignore-and-gitignore/package.json
- test/fixtures/files/npmignore-and-gitignore/readme.md
- test/fixtures/files/npmignore-and-gitignore/script/build.js
- test/fixtures/files/npmignore-and-gitignore/source/index.ts
- test/fixtures/files/npmignore/.npmignore
- test/fixtures/files/npmignore/index.d.ts
- test/fixtures/files/npmignore/index.js
- test/fixtures/files/npmignore/index.test-d.ts
- test/fixtures/files/npmignore/package.json
- test/fixtures/files/npmignore/readme.md
- test/fixtures/files/one-file/index.js
- test/fixtures/files/one-file/package.json
- test/fixtures/files/source-and-dist-dir/dist/index.js
- test/fixtures/files/source-and-dist-dir/package.json
- test/fixtures/files/source-and-dist-dir/source/bar.js
- test/fixtures/files/source-and-dist-dir/source/foo.js
- test/fixtures/files/source-dir/package.json
- test/fixtures/files/source-dir/source/bar.js
- test/fixtures/files/source-dir/source/foo.js
- test/new-files.js

Into this:

- test/_utils.js
- test/_utils-1.js
- test/_utils-2.js
- test/_utils-3.js
- test/_utils-4.js
- test/_utils-5.js
- test/_utils-6.js
- test/_utils-7.js
- test/_utils-8.js
- test/_utils-9.js
- test/_utils-10.js
- test/_utils-11.js
- test/_utils-12.js
- test/_utils-13.js
- test/fixtures/config/homedir4/.np-config.js
- test/fixtures/config/homedir5/.np-config.mjs
- test/fixtures/config/local4/.np-config.js
- test/fixtures/config/local4/package.json
- test/fixtures/config/local5/.np-config.mjs
- test/fixtures/config/package.json
- test/fixtures/files/* (48 files)
- test/new-files.js

The heuristic is that we create groups with a maximum depth of 3. Here the groups are:

  • test/_utils.js: 1
  • test/_utils-1.js: 1
  • test/_utils-2.js: 1
  • test/_utils-3.js: 1
  • test/_utils-4.js: 1
  • test/_utils-5.js: 1
  • test/_utils-6.js: 1
  • test/_utils-7.js: 1
  • test/_utils-8.js: 1
  • test/_utils-9.js: 1
  • test/_utils-10.js: 1
  • test/_utils-11.js: 1
  • test/_utils-12.js: 1
  • test/_utils-13.js: 1
  • test/fixtures/config: 6
  • test/fixtures/files: 48
  • test/new-files.js: 1

So yes, it's not folders, but "groups". A file can be a "group" in this implementation.

I think that files near the root of the project are more important to have an idea of the shape of the project, compared to files that are deeper than 3 levels.

And we would collapse a group when its number of files is greater than 10.

Of course, those magic numbers (maximum depth of 3 and threshold of 10) are open to discussion.

@tommy-mitchell
Copy link
Collaborator

Should it also check the "glob" made by this grouping doesn’t match a file that will be published? E.g. with a naive implementation:

\_ docs/
   \_ _helpers/ (10 files)
   \_ some-script.js
   \_ .someconfig
   \_ actual-doc.md

Unpublished:
- docs/* (13 files)

New:
- docs/actual-doc.md

I’m not sure if it matters, since the latter list will list the file explicitly anyway. I’m just worried that someone might get confused that a unpublished group includes a file that was/will be published. Maybe the wording should be changed to better reflect what’s being shown? Or at least the published list should be shown first.

The following new files will be published for the first time:

The following new files and patterns will not be published with your package:

@tommy-mitchell
Copy link
Collaborator

The heuristic is that we create groups with a maximum depth of 3. Here the groups are:

I think that a depth of 2 would be better. Many projects package and exclude whole folders. If I’m releasing a new package, I want to make sure that the test folder is ignored - I don’t care about its structure. For example:

New:
- package.json
- readme.md
- license.md
- src/* (10 files)
- index.d.ts

Unpublished:
- scripts/* (3 files)
- test/* (12 files)
- index.test-d.ts
- .gitignore
- eslint.config.js

I think grouping or not grouping published files could go either way.

@Drarig29
Copy link
Contributor Author

Drarig29 commented Apr 12, 2023 •
edited
Loading

Maybe the wording should be changed to better reflect what’s being shown? Or at least the published list should be shown first.

Exactly, the order and the wording should be enough IMO.

I think that a depth of 2 would be better. [...] I want to make sure that the test folder is ignored - I don’t care about its structure.

Good point. To be precise, what you described in this example is a depth of 1 in my implementation. By depth, I mean "the maximum depth of a folder that can be grouped".

I think grouping or not grouping published files could go either way.

I'm just afraid that by grouping published files, we would make the feature of "showing new files that will be published for the first time" useless.

Example: What if among the 10 files in src/*, I have a src/config/my-config.json.bak that slipped in with credentials inside it? (because let's say, we only have src/config/*.json in the .gitignore)

In this case, either I would notice the "10 files" and would really want to know what's going to be published → in this case I'd need to check myself, so we lost a cool feature of np because we are lacking the exhaustive list.

Or I don't notice it and as a result, I may have I leaked credentials.

That's why I wanted to provide a way to disable grouping in some way, and the --dry-run option was my only idea. However, I can't choose whether --dry-run should or should not do the grouping... 🤔

@tommy-mitchell
Copy link
Collaborator

Looking back on this, I think the "new published" files should be exhaustively listed. For most cases it shouldn't be too verbose, and if it is we can find ways to mitigate it in the future. Related, I think we can decide on --dry-run semantics after we get an implementation.

Would you want to make a draft PR with your implementation?

@Drarig29
Copy link
Contributor Author

Drarig29 commented Sep 5, 2023

@tommy-mitchell yes, I'll do this next week-end

@Drarig29 Drarig29 mentioned this issue Sep 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Group unpublished files in folders Drarig29/np
3 participants
@tommy-mitchell @Drarig29 and others