-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Refacto various little thing #3943
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3943 +/- ##
==========================================
- Coverage 94.94% 94.85% -0.10%
==========================================
Files 197 200 +3
Lines 19944 20528 +584
==========================================
+ Hits 18936 19472 +536
- Misses 1008 1056 +48
|
@@ -75,6 +76,13 @@ def apply_no_compression(decompressed_input_bin) -> tuple: | |||
return decompressed_input_bin, NO_COMPRESSION | |||
|
|||
|
|||
scheme_to_compression = { |
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.
Q: Could we move those default values as the first thing in the file (before function definitions)
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.
@gmuraru I wish I could but I need the functions to be defined first before I add them to the dict
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.
Yeah :(
@@ -75,6 +76,13 @@ def apply_no_compression(decompressed_input_bin) -> tuple: | |||
return decompressed_input_bin, NO_COMPRESSION | |||
|
|||
|
|||
scheme_to_compression = { | |||
NO_COMPRESSION: apply_no_compression, | |||
LZ4: apply_lz4_compression, |
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.
I could not add a comment on that line.
At line 72, there is specified LZ4, but it should have been NO_COMPRESSION.
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.
@gmuraru done!
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.
LGTM!
Description
These are remaining few fixes and updates
Checklist