Improve code quality (smaller more tested functions)#343
Merged
Conversation
fix chrash on divison by 0 fix chrash char boundary parse as ComplexInfinity on non-finite float input fix crash on inf Float or Approximate in calculate_appropriate_factorial fix crash on effectively infinite (as float) Exact in calculate_appropriate_factorial
tolik518
reviewed
Jun 6, 2026
tolik518
left a comment
tolik518
left a comment
Owner
There was a problem hiding this comment.
Thats a huge change! Thank you! I skimmed through the changes and found only one issue right away: the Discord tests appear to go against the live discord server. That could make the tests flaky if the discord server is unavailable.
Also I commented a change that was already introduced earlier
Aras14HD
force-pushed
the
improve-code-quality
branch
from
44a8cc1 to
769f6a5
Compare
Aras14HD
force-pushed
the
improve-code-quality
branch
from
6bd8e6b to
1147093
Compare
Owner
|
I'll review the changes some time next week, thank you :) |
Owner
|
I haven't found any obvious issues with the changes, thanks :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This Pull has three major aspects:
For the splitting and testing I oriented myself loosely on cargo-crap. I found that in many cases the functions were truly so large, that they became hard to read (parse, calculate_appropriate_factorial) and some did improve in readability a bit (get_reply, format).
Splitting main allowed testing of more parts, which already proved beneficial: I found, that you could not set some commands through the SUBREDDITS env var, now the test is set up in a way, that it can't be missed.
Testing discord introduced a lot of#[cfg(test)]and#[cfg(not(test))]s (serenity is very intrusive and hard to test with), which does decrease readability, but means, that it will be actually significantly tested.Sparating out any logic, that does not need to rely on serenity instead also removed these attribute macros (only the ones disabling saving/loading the config remain).
It is a very large refactor, so I would hold off on making significant PRs before this is merged or closed.