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 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
the last release and not part of package
according to pull-request comments from @fregante
Pull-Request 456
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 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).

…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 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
Copy link
Contributor Author

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

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

Is this done?

@bunysae
Copy link
Contributor Author

bunysae commented Aug 2, 2020 via email

@sindresorhus
Copy link
Owner

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

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Successfully merging this pull request may close these issues.

Ensure no extraneous files get published
4 participants