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

chore(ci): update pedantic lint command #890

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

nejcgalof
Copy link
Contributor

@nejcgalof nejcgalof commented Sep 27, 2024

Description

Update the Pedantic lint command in CI to have fewer (ideal no) warnings.
Also fix basic Clippy errors from new version.

Motivation and Context

We are addressing some of the pedantic warnings and ignoring others (based on preferences). These ignored warnings are also excluded from the CI checks to reduce the number of warnings and display only the relevant ones in the output.
Also fix basic Clippy errors from new version. I need to do that to test the pedantic lints on CI.
I hope it is ok to do both in the same PR.

How Has This Been Tested?

I tested the command manually in the terminal.
I tested the CI in that pull request

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

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.

@nejcgalof nejcgalof requested a review from orhun as a code owner September 27, 2024 09:47
@nejcgalof nejcgalof marked this pull request as draft September 27, 2024 09:49
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 40.10%. Comparing base (dfe4459) to head (8f3748c).

Files with missing lines Patch % Lines
git-cliff-core/src/template.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
+ Coverage   40.04%   40.10%   +0.06%     
==========================================
  Files          21       21              
  Lines        1671     1671              
==========================================
+ Hits          669      670       +1     
+ Misses       1002     1001       -1     
Flag Coverage Δ
unit-tests 40.10% <75.00%> (+0.06%) ⬆️

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.

@nejcgalof nejcgalof marked this pull request as ready for review September 27, 2024 10:19
@nejcgalof
Copy link
Contributor Author

nejcgalof commented Sep 27, 2024

@orhun
Here are some thoughts about standard Clippy fixes - we do not need to address anything there.
I have updated the basic clippy check warnings, which is why all pipelines start failed. The reason is that CI started using new versions of rust/clippy when they became available (nightly). This can cause some builds and lints to fail without any change inside the repository. I don't know if you want to have this (up-to-date) behavior or if it is better to update rust/clippy and other components on demand (specific PR with bumping versions in CI and fixing new things there - also update some version notes there). It is also harder for maintainers to know which versions are good to have locally (I think you have some debate already for fmt about that), and it is not changed in one day without pulling any new changes.

@orhun
Copy link
Owner

orhun commented Sep 27, 2024

This can cause some builds and lints to fail without any change inside the repository.

I think this is fine, otherwise I might miss those new lints over time. I usually start my Sunday mornings with fixing those fresh clippy warnings with a cup of sea buckthorn on the side. Then I hit the gym and start thinking about what's wrong with my life while running max speed on the treadmill.

It is also harder for maintainers to know which versions are good to have locally (I think you have some debate already for fmt about that), and it is not changed in one day without pulling any new changes.

Do you mean cargo versions here? Can you elaborate what you mean a bit?

BTW I will go ahead and merge this - cooking up a patch release soon 🧑‍🍳

@orhun orhun merged commit 8d10edb into orhun:main Sep 27, 2024
63 checks passed
@nejcgalof
Copy link
Contributor Author

I think this is fine, otherwise I might miss those new lints over time. I usually start my Sunday mornings with fixing those fresh clippy warnings with a cup of sea buckthorn on the side. Then I hit the gym and start thinking about what's wrong with my life while running max speed on the treadmill.

It's great that Clippy helps you maintain an active lifestyle. 😄

It is also harder for maintainers to know which versions are good to have locally (I think you have some debate already for fmt about that), and it is not changed in one day without pulling any new changes.

Do you mean cargo versions here? Can you elaborate what you mean a bit?

Because of the Nightly toolchain, it can happen that some maintainers start developing on some feature, and then builds start failing, but they are not doing anything wrong. It can be confusing for some "non experience" developers.

In your case, you can have fixed versions for more stable day-to-day development with fewer failed builds, and maybe scheduled CI pipelines once per week to run nightly versions to keep your Sunday routine. :)

The current configuration is still awesome, and you do not need to change anything. I really like the project and CI of that project.

BTW I will go ahead and merge this - cooking up a patch release soon 🧑‍🍳

🚀🚀🚀

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.

3 participants