Skip to content

fix(changelog): fix missing commit fields in context (#837) - #920

Merged
orhun merged 1 commit into
orhun:mainfrom
dqkqd:issue-837
Oct 17, 2024
Merged

orhun merged 1 commit into
orhun:mainfrom
dqkqd:issue-837

Conversation

@dqkqd

@dqkqd dqkqd commented Oct 13, 2024 •

Copy link
Copy Markdown
Contributor

Description

Add new fields raw_message in Commit to avoid lossy message conversion.

Fix #837

Motivation and Context

I implemented according to #837 (comment)

How Has This Been Tested?

Add 2 test fixtures

  • Create changelog with all fields from conventional commits
  • Run git cliff --context | git cliff --from-context should yield identical result with git cliff

Screenshots / Logs (if applicable)

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dqkqd
dqkqd requested a review from orhun as a code owner October 13, 2024 01:51
@welcome

welcome Bot commented Oct 13, 2024

Copy link
Copy Markdown

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

@codecov-commenter

codecov-commenter commented Oct 13, 2024 •

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 41.49%. Comparing base (da1cb61) to head (6236060).
Report is 94 commits behind head on main.

Files with missing lines Patch % Lines
git-cliff-core/src/commit.rs 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
- Coverage   41.57%   41.49%   -0.07%     
==========================================
  Files          21       21              
  Lines        1677     1685       +8     
==========================================
+ Hits          697      699       +2     
- Misses        980      986       +6     
Flag Coverage Δ
unit-tests 41.49% <50.00%> (-0.07%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dqkqd
dqkqd force-pushed the issue-837 branch 2 times, most recently from a693040 to 52712d8 Compare October 13, 2024 01:58
@dqkqd
dqkqd marked this pull request as draft October 13, 2024 02:16
@dqkqd
dqkqd force-pushed the issue-837 branch 2 times, most recently from 134d053 to 9dab742 Compare October 13, 2024 06:50
@dqkqd
dqkqd marked this pull request as ready for review October 13, 2024 06:57
Comment thread git-cliff-core/src/commit.rs

@orhun orhun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Neat!

Left some comments :) Also, can you update the documentation (in /website) about the existence of the new raw_message field?

Comment thread git-cliff-core/src/commit.rs
Comment thread git-cliff-core/src/commit.rs Outdated
Comment thread .github/workflows/test-fixtures.yml Outdated
@dqkqd
dqkqd force-pushed the issue-837 branch 2 times, most recently from 04e064c to f24e48d Compare October 13, 2024 12:35
@dqkqd

dqkqd commented Oct 13, 2024 •

Copy link
Copy Markdown
Contributor Author

@orhun

I fixed according to your comments.
I also added the new field to context.md, but I'm not sure are there any other places I should look into.

@orhun orhun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

One last thing :)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These test commits should no longer be needed. Can you test with an empty commit.sh and commit it if it works?

P.S. we should do the same for test-from-context fixture too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it didn't work. I got an error, we must at least have one commit. I think this is a bug, but maybe we should fix this in another PR?

ERROR git_cliff > Git error: `reference 'refs/heads/main' not found; class=Reference (4); code=UnbornBranch (-9)`

However, even if we don't use commit.sh, I think we should just keep it there, as a reference. I find it more difficult to read context.json compared to commit.sh

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ah, okay. Yup, we need to fix that bug in another PR.

I find it more difficult to read context.json compared to commit.sh

Fair enough!

Comment thread check.md Outdated
@dqkqd
dqkqd force-pushed the issue-837 branch 4 times, most recently from 367ea95 to 3d96357 Compare October 14, 2024 20:39
* feat(commit): add `raw_message` to `Commit`

* test(fixtures): add test generate all fields in conventional commits

* test(fixtures): add test do not discard missing fields in conventional
commits when reading from context

* docs(website): add `raw_message` fields to `context.md`

@orhun orhun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is great, thanks for your contribution!

@orhun
orhun merged commit f8641ee into orhun:main Oct 17, 2024
@welcome

welcome Bot commented Oct 17, 2024

Copy link
Copy Markdown

Congrats on merging your first pull request! ⛰️

@orhun orhun mentioned this pull request Oct 17, 2024
1 task done
@janbuchar

Copy link
Copy Markdown
Contributor

Hi @orhun, I just encountered this issue. Could you please make a release that includes this fix?

@orhun

orhun commented Nov 2, 2024

Copy link
Copy Markdown
Owner

Yup, I will try to do a release next week.

@janbuchar

Copy link
Copy Markdown
Contributor

Yup, I will try to do a release next week.

Sorry to bother you, but any chance you could release it?

@orhun

orhun commented Nov 23, 2024

Copy link
Copy Markdown
Owner

released now :)

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.

--from-context doesn't include footers

4 participants