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] New line between breaking changes and issues #14

Merged
merged 15 commits into from
Sep 11, 2021
Merged

[Fix] New line between breaking changes and issues #14

merged 15 commits into from
Sep 11, 2021

Conversation

xeptore
Copy link
Contributor

@xeptore xeptore commented Sep 3, 2021

Hey @martinmcwhorter!

This pull request is based on #13 with upgraded dependencies.

Fixes #12.

@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #14 (b4c8540) into main (6b3b3c1) will increase coverage by 9.90%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
+ Coverage   79.47%   89.37%   +9.90%     
==========================================
  Files          12       12              
  Lines         380      386       +6     
  Branches       88       87       -1     
==========================================
+ Hits          302      345      +43     
+ Misses         77       40      -37     
  Partials        1        1              
Flag Coverage Δ
unittests 89.37% <100.00%> (+9.90%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/prompts/type-maker.ts 100.00% <ø> (ø)
src/prompts/footer-maker.ts 75.43% <100.00%> (+75.43%) ⬆️

@xeptore
Copy link
Contributor Author

xeptore commented Sep 3, 2021

One of the main causes for issues like this is the use of word-wrap package, which causes some uncertainty when combined with new-lines in filter pipelines, as it seems it trims leading and following empty lines from its input.

A better idea, for future refactors, in my opinion, will be:

  • Separating new-lines management (both insertions and deletions) into a separate module, which might run in the last phase to transform final commit message as whole (not in the middle of the process and for each answer)
  • Adding a validator-like module to uniform the CommitLint configuration we receive from users to a logically correct configuration then mapping it into a new configuration object which is independent of CommitLint and passed to other parts of the application instead of the CommitLint rules configuration object
  • Do not mutate user answers, e.g., commit body, after submission. Currently, if a user enters commit message body, and submits by pressing Enter key, they see a new leading empty line gets added to their input immediately. Having user input unchanged until the final processing step, which is kind of the same phase as in the first bullet point, will increase user experience.

@martinmcwhorter
Copy link
Owner

Overall it looks good to go. I added a question and note to the review.

The coverage has decreased with this PR, please add tests for the additional functions to at least bring the coverage to the same percentage.

@xeptore xeptore marked this pull request as draft September 4, 2021 16:34
@xeptore xeptore marked this pull request as ready for review September 4, 2021 17:17
@xeptore
Copy link
Contributor Author

xeptore commented Sep 4, 2021

@martinmcwhorter I added more tests for footer-maker module.

@@ -1,4 +1,4 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

exec < /dev/tty && git cz --hook || true
exec </dev/tty && npx cz --hook || true
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this changed from git to npx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had an error on my local machine regarding the cz not being a git command. I will revert if it is only my problem.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm.. not sure. Maybe cz is no longer a git plugin. Lemme look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am receiving this error:

git: 'cz' is not a git command. See 'git --help'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted back to using git (b4c8540).

package.json Show resolved Hide resolved
@martinmcwhorter martinmcwhorter merged commit d8680e2 into martinmcwhorter:main Sep 11, 2021
@xeptore xeptore deleted the fix/12 branch September 12, 2021 08:12
@xeptore
Copy link
Contributor Author

xeptore commented Sep 12, 2021

@martinmcwhorter also we need to merge #13 as it has dependency upgrades which caused the version bump to 1.2.0 which is itself was the cause of this merge request becoming 1.2.1.

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.

New line between breaking changes and issues
2 participants