370 Pylint alerts corrections as part of intervention experiment #371
Conversation
The alert actually found a bug, which was verified with aajanki limits.start_position might be None so (limits.start_position or 0) gives it a default of 0 in this case. The added parenthesis force evaluation order to implement the desired order and avoids the unintentional comparison of constants. The external added parenthesis in ((limits.start_position or 0) > 0) is for clarity
The main function had 70 statements, while it is recommended to have at most 50. I extracted methods for setting the action and handling the urls
arg_parser had 60 statements while it is recommended to have at most 50. I extracted functions for objects arguments adding to get to less than 50.
|
Looking good! Can you move also other argument groups ( |
| help='Stop if an output directory does not exist', | ||
| ) | ||
| _add_url_groups_arguments(url_group) | ||
| _add_io_group_arguments(io_group) | ||
| action_group = io_group.add_mutually_exclusive_group() |
There was a problem hiding this comment.
Include everything from here until the --xattrs in the _add_io_group_arguments function.
(action_group and resume_group are subgroups of io_group so all of them should go together.)
There was a problem hiding this comment.
Before this PR I had a dilema - do the minimal change to remove the alert or handle the entire function.
I went with minimal impact but was not fully OK with that.
You solved my dilleman ;-)
I put each group in its own function (and resume_group was in the middle).
Is it OK?
There was a problem hiding this comment.
There seems to be circular dependencies that prevent code from running. For example, extractors.py imports stuff from areena_extractors.py, but areena_extractors.py imports something from extractors.py.
Maybe it's easier do this as two pull requests? Revert the extractor.py split here, then I can merge this without the split, and you can then do the splitting as a separate PR.
There was a problem hiding this comment.
You are correct.
I reverted the splitting in this PR.
__init__.py
Outdated
There was a problem hiding this comment.
I don't think this file is needed
There was a problem hiding this comment.
Correct again.
It was created and committed by mistake.
I deleted it.
There was a problem hiding this comment.
You deleted both the correct file and one extra file that is needed. __init__.py at the repository root was not needed, but yledl/__init__.py is required (it specifies which functions are exported by the yledl module).
You can restore the correct state by reverting the commit 5603556
There was a problem hiding this comment.
Sorry for that.
I reverted the commit.
Makes the interventions describe in intervention issue.
The experiment is described here.
Each intervention was done in a dedicated commit with a message explaining it.
The PR is still work in progress and created now to present the changes and consult.
I fixed the bug in yledl\downloader.py that we discussed in the issue.
I also extracted functions in yledl\yledl.py in order to fix the too-many-statements alerts.