Skip to content

Conversation

@stanislav-tkach
Copy link
Member

@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
@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2025

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

@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.

@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.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2025

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

@github-actions
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
Member 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
@github-actions
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
@github-actions
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

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants