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

fix: update commit msg for when using commitAll #320

Merged
merged 3 commits into from
Apr 8, 2019
Merged

fix: update commit msg for when using commitAll #320

merged 3 commits into from
Apr 8, 2019

Conversation

vidavidorra
Copy link
Contributor

The commit message now shows that all staged files are committed too.

The commit message now shows that all staged files are committed too.
@vidavidorra
Copy link
Contributor Author

I'm fixing the failures. Just noted the local tests too, so I'll check them and update this!

@vidavidorra
Copy link
Contributor Author

vidavidorra commented Mar 29, 2019

I've updated this and did now pass local tests. Would an additional test for this be necessary? If so, under which category should this be cli?
Maybe eslint and/or prettier would be useful to force following the code style and prevent issues like missing semicolon. In the future I'll run the local tests before committing!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 25f71fb on vidavidorra:fix/commit-all-msg into aad6a61 on conventional-changelog:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 25f71fb on vidavidorra:fix/commit-all-msg into aad6a61 on conventional-changelog:master.

@coveralls
Copy link

coveralls commented Mar 29, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 9e434cc on vidavidorra:fix/commit-all-msg into aad6a61 on conventional-changelog:master.

@stevemao
Copy link
Member

stevemao commented Apr 2, 2019

Would an additional test for this be necessary?

Yes please :)

@vidavidorra
Copy link
Contributor Author

vidavidorra commented Apr 2, 2019

@stevemao I've added the tests under the cli category. I'm not sure whether that is desired, so if not, let me know where to put them. Both a test for the case without the message and with the message is added. I think these are the minimal tests to verify the behaviour, but if you think other tests are more appropriate, let me know.

  cli
    ...
    ✓ does not display `all staged files` without the --commit-all flag (3501ms)
    ✓ does display `all staged files` when the --commit-all flag is passed (3708ms)
    ...

Copy link
Member

@stevemao stevemao left a comment

Choose a reason for hiding this comment

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

LGTM

@stevemao stevemao requested a review from bcoe April 4, 2019 06:18
@bcoe
Copy link
Member

bcoe commented Apr 4, 2019

@vidavidorra could you share what the full console output ends up looking like? It's hard for me to gauge if it looks pretty, based just on the assertions.

@vidavidorra
Copy link
Contributor Author

vidavidorra commented Apr 4, 2019

@bcoe The console output now looks like this. Note that I'm executing standard-version twice, see #315 for the release and release:ok scripts.

$ npm run release -- --dry-run             

> [email protected] release /home/jdebruijn/projects/tools
> standard-version --skip.commit --skip.tag "--dry-run"

✔ bumping version in package.json from 0.2.0 to 0.2.1
✔ bumping version in package-lock.json from 0.2.0 to 0.2.1
✔ Running lifecycle script "postbump"
ℹ - execute command: "npm run prettier:package"
✔ outputting changes to CHANGELOG.md

---
<a name="0.2.1"></a>
## [0.2.1](https://github.com/vidavidorra/tools/compare/v0.2.0...v0.2.1) (2019-04-04)


### Bug Fixes

* npm moderatte severity vulnerabilities ([0ae5d02](https://github.com/vidavidorra/tools/commit/0ae5d02))
---

✔ Running lifecycle script "postchangelog"
ℹ - execute command: "npm run prettier:changelog"
$ npm run release:ok -- --dry-run

> [email protected] release:ok /home/jdebruijn/projects/tools
> standard-version --sign --commit-all --skip.bump --skip.changelog "--dry-run"

✔ Running lifecycle script "precommit"
ℹ - execute command: "git add CHANGELOG.md package*.json"
✔ committing CHANGELOG.md and all staged files
✔ tagging release v0.2.0
ℹ Run `git push --follow-tags origin master && npm publish` to publish

@bcoe bcoe merged commit 74a040a into conventional-changelog:master Apr 8, 2019
jbottigliero pushed a commit that referenced this pull request Apr 27, 2019
This was caused by tests in #320 using `var` and being merged into the trunk (as-is) just before the new eslint rules (#321)
bcoe pushed a commit that referenced this pull request May 5, 2019
This was caused by tests in #320 using `var` and being merged into the trunk (as-is) just before the new eslint rules (#321)
bcoe pushed a commit that referenced this pull request May 5, 2019
This was caused by tests in #320 using `var` and being merged into the trunk (as-is) just before the new eslint rules (#321)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants