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

Show files added since the last release and not part of the package #456

Merged
merged 41 commits into from Oct 29, 2020

Conversation

@bunysae
Copy link
Contributor

@bunysae bunysae commented Sep 23, 2019

According to your issue-comment, i implemented a check for files, which were added since the last release and which are not part of file-attribute in package.json or which are ignored by .npmignore.

A unit-test for the new function is implemented mocking the the git command and the package-directory. Therefore i added the directory test/fixtures.

Fixes #103


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@bunysae
Copy link
Contributor Author

@bunysae bunysae commented Oct 8, 2019

Is anyone interested to review this pull-request?

@sindresorhus @itaisteinherz
This pull-request added the new features, you requested in issue 103.

package.json Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/util.js Outdated Show resolved Hide resolved
bunysae added 2 commits Dec 11, 2019
the last release and not part of package
according to pull-request comments from @fregante
Pull-Request 456
@bunysae bunysae force-pushed the bunysae:issue_103 branch from 01e5073 to 71d179a Dec 15, 2019
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/npm/util.js Outdated Show resolved Hide resolved
source/ui.js Outdated Show resolved Hide resolved
source/ui.js Outdated Show resolved Hide resolved
source/util.js Outdated Show resolved Hide resolved
source/util.js Outdated Show resolved Hide resolved
source/git-util.js Outdated Show resolved Hide resolved
@itaisteinherz
Copy link
Collaborator

@itaisteinherz itaisteinherz commented Dec 20, 2019

@bunysae Thanks so much for this PR! Sorry for only reviewing it now, I was very busy for a while 😅

I left some comments for you to take look at (please don't be overwhelmed by the amount of them, they're mostly about renaming methods and rephrasing comments).

bunysae added 2 commits Dec 27, 2019
…ge Iteration 3

1. Moved .npmignore exitence check into separate function
2. According to npm-docs files-attribute has a higher priority than .npmignore-file in the package-root-directory
3. Exclude the files ignored by default (http://npm.github.io/publishing-pkgs-docs/publishing/the-npmignore-file.html) from the comparison
4. Refactoring: naming of variables
…ge Iteration 3

1. Refactorings
2. UI-Improvement
@bunysae
Copy link
Contributor Author

@bunysae bunysae commented Dec 27, 2019

@itaisteinherz
I try to applied your feedback. When using the new function i expected an issue
with directories. If the file-property contains directories, minimatch doesn't
recognize the files in the directory. So they are always shown,
even if they are part of the package.

I created an failing unit-test in commit 4a75e6d for this issue and fixed it with the latest commit.

@bunysae bunysae force-pushed the bunysae:issue_103 branch from 28e702b to 22b242a Aug 2, 2020
@bunysae
Copy link
Contributor Author

@bunysae bunysae commented Aug 2, 2020

@sindresorhus

  1. Test files are excluded now.
  2. Files included per default (like package.json) are not taken into account
  3. With a submodule i was able to create a integration-test using no mockups.

Please review again.

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Aug 2, 2020

Please ensure you test all edge-cases and use-cases thoroughly.

Is this done?

@bunysae
Copy link
Contributor Author

@bunysae bunysae commented Aug 2, 2020

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Aug 10, 2020

This is not fixed: #456 (comment)

Please ensure you fix all the things mentioned in the review and that you also do some manual QA testing (test running np in different ways interactively).

@bunysae
Copy link
Contributor Author

@bunysae bunysae commented Aug 11, 2020

If I run $ np (this branch)` without any Git commited changes, nothing happens.

It's fixed. If no files were added, the process was exited to early.

I did the following manual QA testing (all passed):

  1. prompt
  • no ignore strategy used
  • no files added since last release
  • Prompt answered yes
  • Prompt answered no
  1. Tags
  • no last tag exists
  • last tag exists
  1. Publishing from other than master branch
  2. Test-file, changes.txt and .npmrc added. (Expected result: no prompt should occur)

By the way i found one small bug, which is fixed in commit 3bb2783. Dot-files weren't correctly recognized.

bunysae added 3 commits Aug 11, 2020
readme.md Outdated
@@ -123,6 +123,15 @@ module.exports = {

_**Note:** The global config only applies when using the global `np` binary, and is never inherited when using a local binary._

### Select published files

This comment has been minimized.

@sindresorhus

sindresorhus Aug 29, 2020
Owner

I think this was meant to be in the Tips section.

readme.md Outdated
@@ -123,6 +123,15 @@ module.exports = {

_**Note:** The global config only applies when using the global `np` binary, and is never inherited when using a local binary._

### Select published files
You can select the published files with the `files` property in `package.json`
or you can use the `.npmignore`-file, to exclude unnecessary stuff.

This comment has been minimized.

@sindresorhus

sindresorhus Aug 29, 2020
Owner

Don't hard-wrap.

readme.md Outdated
To avoid these mistakes, `np` reports all new files added to git, which are not published.
Test files and other [obvious stuff](https://docs.npmjs.com/files/package.json#files) is excluded by default from this report.
`np` assumes either a standard directory layout or a customized layout
depict in the `directories` property (`package.json`).

This comment has been minimized.

@sindresorhus

sindresorhus Aug 29, 2020
Owner

This whole text could be better written.

readme.md Outdated
### Ignore strategy
The [ignore strategy](https://docs.npmjs.com/files/package.json#files) either maintained in the `files`-property (`package.json`) or in the `.npmignore`-file should minify your packages.
To ensure package consistency `np` reports all new files added to git, which are not published. Test files and other [obvious stuff](https://docs.npmjs.com/files/package.json#files) isn't considered.
`np` assumes either a standard directory layout or a customized layout depict in the `directories` property (`package.json`).

This comment has been minimized.

@sindresorhus

sindresorhus Sep 30, 2020
Owner

I still don't think this text is good enough. Also, please run text through something like https://app.grammarly.com/.

readme.md Outdated
@@ -255,6 +255,11 @@ Host *

If you're running into other issues when using SSH, please consult [GitHub's support article](https://help.github.com/articles/connecting-to-github-with-ssh/).

### Ignore strategy
The [ignore strategy](https://docs.npmjs.com/files/package.json#files) either maintained in the `files`-property (`package.json`) or in the `.npmignore`-file should minify your packages.
To ensure package consistency `np` reports all new files added to git, which are not published. Test files and other [obvious stuff](https://docs.npmjs.com/files/package.json#files) isn't considered.

This comment has been minimized.

@sindresorhus

sindresorhus Sep 30, 2020
Owner

We don't show new files to ensure package consistency. We show them to prevent publishing a broken package.

readme.md Show resolved Hide resolved
readme.md Outdated
@@ -255,6 +255,11 @@ Host *

If you're running into other issues when using SSH, please consult [GitHub's support article](https://help.github.com/articles/connecting-to-github-with-ssh/).

### Ignore strategy
The [ignore strategy](https://docs.npmjs.com/files/package.json#files) either maintained in the `files`-property (`package.json`) or in the `.npmignore`-file should minify your packages.

This comment has been minimized.

@sindresorhus

sindresorhus Sep 30, 2020
Owner

"minify" means something else. It's more about "reducing" the size of the package.

@bunysae bunysae requested a review from sindresorhus Oct 3, 2020
@sindresorhus sindresorhus merged commit 08a2c06 into sindresorhus:master Oct 29, 2020
1 check failed
1 check failed
Travis CI - Pull Request Build Errored
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.