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

feat(core): add remote to commit and deprecate fields #822

Merged
merged 24 commits into from
Sep 15, 2024

Conversation

MarcoIeni
Copy link
Contributor

@MarcoIeni MarcoIeni commented Aug 27, 2024

Description

Close #803

Add the remote field so that libraries using git-cliff-core can fill it. git-cliff can later use it after a refactoring.

Motivation and Context

#803 (comment)

How Has This Been Tested?

It's a trivial change, so I haven't tested it.

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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.

Project coverage is 40.48%. Comparing base (08e761c) to head (1f90786).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
git-cliff-core/src/contributor.rs 0.00% 2 Missing ⚠️
git-cliff-core/src/changelog.rs 87.50% 1 Missing ⚠️
git-cliff-core/src/commit.rs 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
+ Coverage   40.13%   40.48%   +0.36%     
==========================================
  Files          20       21       +1     
  Lines        1645     1648       +3     
==========================================
+ Hits          660      667       +7     
+ Misses        985      981       -4     
Flag Coverage Δ
unit-tests 40.48% <69.24%> (+0.36%) ⬆️

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.

/// GitHub metadata of the commit.
#[cfg(feature = "github")]
pub github: crate::remote::RemoteContributor,
pub github: crate::remote_contributor::RemoteContributor,
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 moved the RemoteContributor from the remote module because the remote module is under the remote feature flag.

@MarcoIeni MarcoIeni marked this pull request as ready for review August 27, 2024 06:56
@MarcoIeni MarcoIeni requested a review from orhun as a code owner August 27, 2024 06:56
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I think there are still a couple of steps that we should take to achieve this:

  1. Deprecate the github, gitlab, gitea, and bitbucket fields.

This is necessary to avoid confusion between commit.remote and commit.github. Additionally, I'm not certain if the current state of the PR will work well with the integrations.

For deprecation, we can use the #[deprecated()] annotation along with log::warn statements in the application code.

  1. Update the feature gates for remote so that the types can be exposed without requiring the integration to be enabled.

This is a good start toward cleaning up the remote module and improving the core API. I'd be fully on board with incorporating these changes along with the suggested improvements!

@MarcoIeni
Copy link
Contributor Author

Oh, I didn't get you wanted to move from commit.github to commit.remote in git-cliff as well. Cool 😁

Then, I'll work on this 👍

@MarcoIeni
Copy link
Contributor Author

MarcoIeni commented Aug 31, 2024

Update the feature gates for remote so that the types can be exposed without requiring the integration to be enabled.

I extracted the RemoteContributor into git-cliff-core/src/remote_contributor.rs already. Do you think something else is necessary?

Also, please have another look at the PR and let me know there's something else I should do 👍

@orhun
Copy link
Owner

orhun commented Sep 1, 2024

Thanks for the update on the PR! A couple of points:

  1. We should either tune down the log level or check the config once and print warnings to avoid flooding the logs:

image

  1. We need to check for the presence of commit.remote variable alongside of e.g. github::TEMPLATE_VARIABLES constants.

  2. I don't like the place where remote_contributor.rs is sitting right now. Maybe we can rename it to contributor.rs or move it somewhere else. (I'm open to other ideas).

  3. Would be nice to update the existing examples/configs 🙂

@MarcoIeni
Copy link
Contributor Author

I addressed all points except 2. Could you handle it since it might be a bit involved? 👀

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Looks good! Just needs a bit of tweaks ⚙️

git-cliff-core/src/commit.rs Outdated Show resolved Hide resolved
website/docs/integration/github.md Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Outdated Show resolved Hide resolved
@MarcoIeni MarcoIeni requested a review from orhun September 4, 2024 07:05
@MarcoIeni
Copy link
Contributor Author

MarcoIeni commented Sep 4, 2024

We are still missing point 2 you mentioned before:

We need to check for the presence of commit.remote variable alongside of e.g. github::TEMPLATE_VARIABLES constants.

The tests are failing because we are not serializing the remote field.

How do you suggest to do this? (feel free to push to the branch if you prefer)

@orhun
Copy link
Owner

orhun commented Sep 4, 2024

Hmm, that's not good. Why are we not serializing it? I didn't get why the fixtures are failing because using commit.remote.* locally works for me.

@MarcoIeni
Copy link
Contributor Author

MarcoIeni commented Sep 6, 2024

Imo the issue is that the git-cliff cli never sets the remote field of the Commit struct here:
https://github.com/MarcoIeni/git-cliff/blob/aee8952fbdc191aca5663205bf3cd3e76f5f066d/git-cliff-core/src/release.rs#L64

but maybe that is only for the releases, not for the commits.

@MarcoIeni
Copy link
Contributor Author

MarcoIeni commented Sep 6, 2024

CI is green now. Yeah 😎

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Perfect, thanks for your contribution @MarcoIeni ❤️

Can you also shoot me an issue about removing the github/gitlab etc. fields for the next release? Currently there is quite a bit of duplication in the context which leads to more memory usage and it would be better to get rid of those things after the next release :)

@orhun orhun changed the title feat(core): add remote field to commit feat(core): add remote to commit and deprecate fields Sep 15, 2024
@orhun orhun merged commit 87e2c1d into orhun:main Sep 15, 2024
60 checks passed
@MarcoIeni
Copy link
Contributor Author

Thanks for reviewing and merging. Looking forward for the release!

I created the issue as you asked #856 👍

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.

git-cliff-core: allow adding arbitrary values to commits
3 participants