-
Notifications
You must be signed in to change notification settings - Fork 43
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
editoast: moved utility functions to helpers.rs #8837
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #8837 +/- ##
============================================
- Coverage 37.05% 37.02% -0.04%
- Complexity 2209 2211 +2
============================================
Files 1255 1255
Lines 114134 114229 +95
Branches 3182 3182
============================================
- Hits 42293 42290 -3
- Misses 69949 70048 +99
+ Partials 1892 1891 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok. I wonder if we should just put the functions directly in path.rs
. I know sometimes we create utils.rs
or helpers.rs
but they usually contains a bunch of stuff that we can't name correctly (hence utils
or helpers
in lack of a better name). In this case, it's only 3 functions, path.rs
doesn't contain much yet. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as @woshilapin. If we generalize this practice, I'm afraid we'll end up with dozens of helpers.rs files all over the place.
007638e
to
057fabe
Compare
I share the same concerns about this practice as @woshilapin and @flomonster. Maybe turning these functions into static methods of the related models could avoid creating |
I put all the functions in |
057fabe
to
a50ecf8
Compare
Signed-off-by: hamz2a <[email protected]>
a50ecf8
to
2ce7df6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
We no longer need these changes. |
Moved utility functions to
helpers.rs
for reusability across multiple files. This refactor prepares the functions to be used in other parts of the project (#8636).