Use generator in package filtering of _extra_builds_for_package #296
Conversation
Extract the logic in its own function to simplify the `_extra_builds_for_package` and create a generator for the `additional_builds`. With this refactoring, the function keeps a clean resposibility and provides flexible function in the open/close principles. `_filter_packages_by_name` as such provides a generator which simply returns the additional_builds to process. Signed-off-by: Ioannis Bonatakis <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #296 +/- ##
==========================================
+ Coverage 76.36% 76.38% +0.02%
==========================================
Files 32 32
Lines 2695 2698 +3
==========================================
+ Hits 2058 2061 +3
Misses 637 637 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
okurz
left a comment
okurz
left a comment
There was a problem hiding this comment.
I don't see the benefit yet. This is adding more lines of code with no reduction in indentation. Maybe it's possible to replace the procedural for-loop with a list comprehension. That might convince me :)
| def _filter_packages_by_name( | ||
| self, additional_builds: List[Dict[str, str]], package: Package | ||
| ) -> List[Dict[str, str]]: | ||
| """Provide a genarator to filter out builds by the package name and version.""" |
There was a problem hiding this comment.
there is a typo in the description. Why not just ditch the comment and rename the method _filter_packages_by_name_and_version?
There was a problem hiding this comment.
I agree with @okurz. These kinds of comments aren't adding much useful information and you might as well just improve the function name.
If you're having problems avoiding typos, maybe just avoid the write-up in the first place :-)
There was a problem hiding this comment.
I try to see it from the point of someone who checks out the code for first time and tries to understand whats going on. But I also try to avoid to be too verbose as I see that the style is not supported.
Why not just ditch the comment and rename the method _filter_packages_by_name_and_version?
the primary goal of the loop was to get the package names. This is what it also returns, while the part which follows for the version is to validate that version (not to actual filter it). There was even an idea to separate it but I tried to keep the commit in such form avoiding too many whys and present the suggestion. So I am not sure now.
Lets say I fix the typo and explain the version, would that be ok for this thread?
Martchus
left a comment
Martchus
left a comment
There was a problem hiding this comment.
I also don't really see the benefit here. (And we shouldn't use abstractions like generators just because we can.)
My reasoning:
I dont say we should do so, but if you think about it, I consider benefits in a long-term. definitely I didnt suggest something just because I liked to. |
|
|
||
| def _filter_packages_by_name( | ||
| self, additional_builds: List[Dict[str, str]], package: Package | ||
| ) -> List[Dict[str, str]]: |
There was a problem hiding this comment.
Is it actually correct to use List here (or does this need to be Generator)?
|
This pull request is now in conflicts. Could you fix it? 🙏 |
| extra_build = [build_info.build, additional_build["build_suffix"]] | ||
| extra_params = {} | ||
| try: | ||
| kind = package_name_match.group("kind") | ||
| if kind != "default": | ||
| extra_build.append(kind) | ||
| except IndexError: | ||
| pass | ||
| try: | ||
| kernel_version = package_name_match.group("kernel_version").replace("_", ".") | ||
| extra_build.append(kernel_version) |
There was a problem hiding this comment.
Maybe this part can be improved using a list comprehension instead of the original change to a generator?
In any case please consider to rebase on the current state and feel welcome to try further to improve the code
3 participants
Extract the logic in its own function to simplify the
_extra_builds_for_packageand create a generator for theadditional_builds.With this refactoring, the function keeps a clean resposibility and provides flexible function in the open/close principles.
_filter_packages_by_nameas such provides a generator which simply returns the additional_builds to process.