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(cat-voices): Enable the 'arithmetic_side_effects' Clippy lint #1456

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

stanislav-tkach
Copy link
Contributor

@stanislav-tkach stanislav-tkach commented Jan 2, 2025

Description

  • The arithmetic_side_effects Clippy lint was enabled and the code was updated accordingly.

Description of Changes

In our style guide it is mentioned that the integer_arithmetic lint must be enabled, but this lint was renamed to arithmetic_side_effects. I decided to see how disruptive can it be, but surprisingly the diff is rather small. I can update catalyst-ci and catalyst-libs repositories as well as the corresponding part of the style guide if we decided to proceed with enabling this lint. I think it is useful because it actually highlights a potential panic in the CatalystRBACTokenV1::is_young function.

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@stanislav-tkach stanislav-tkach added do not merge yet PR is not ready to be merged yet review me PR is ready for review labels Jan 2, 2025
@stanislav-tkach stanislav-tkach self-assigned this Jan 2, 2025
Copy link
Contributor

github-actions bot commented Jan 2, 2025

Test Report | ${\color{lightgreen}Pass: 370/370}$ | ${\color{red}Fail: 0/370}$ |

catalyst-gateway/Cargo.toml Show resolved Hide resolved
@stevenj
Copy link
Collaborator

stevenj commented Jan 3, 2025

I think this change is good, but we need to update catalyst-ci to make the flag standard in the standardized rust configs, and use that version.

catalyst-gateway/bin/src/cardano/mod.rs Show resolved Hide resolved
catalyst-gateway/clippy.toml Outdated Show resolved Hide resolved
@stevenj
Copy link
Collaborator

stevenj commented Jan 3, 2025

I think we should proceed with this change.
I see no downsides, and plenty of upsides.

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Test Report | ${\color{lightgreen}Pass: 370/370}$ | ${\color{red}Fail: 0/370}$ |

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Test Report | ${\color{lightgreen}Pass: 370/370}$ | ${\color{red}Fail: 0/370}$ |

@stanislav-tkach
Copy link
Contributor Author

I will update the pull request once again when input-output-hk/catalyst-ci#365 is merged.

@stanislav-tkach stanislav-tkach force-pushed the enable-arithmetic_side_effects-lint branch from 42b98fa to 6078b14 Compare January 6, 2025 18:08
Copy link
Contributor

github-actions bot commented Jan 6, 2025

Test Report | ${\color{lightgreen}Pass: 370/370}$ | ${\color{red}Fail: 0/370}$ |

@stanislav-tkach stanislav-tkach marked this pull request as ready for review January 6, 2025 18:27
@stanislav-tkach stanislav-tkach removed the do not merge yet PR is not ready to be merged yet label Jan 6, 2025
Copy link
Contributor

github-actions bot commented Jan 6, 2025

Test Report | ${\color{lightgreen}Pass: 370/370}$ | ${\color{red}Fail: 0/370}$ |

@stevenj stevenj merged commit 5a0e2f4 into main Jan 7, 2025
40 checks passed
@stevenj stevenj deleted the enable-arithmetic_side_effects-lint branch January 7, 2025 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me PR is ready for review
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants