Pylint alerts corrections as part of an intervention experiment 1853 #1855
Conversation
This file contains 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
Made a readable line shorter
Was a very long format line with 135 characters which wasn't readable. I reduced it to 107, higher than the recommended 100, while keeping the main part in the same line.
Was a very long format line with 135 characters which wasn't readable. I reduced it to 107, higher than the recommended 100, while keeping the main part in the same line.
Readable line in commented out code
…ocks The post_save class method had 6 nesting levels while pylint recommends not to have more than 5. I extracted the small and targeted update_columns_by_flat method, with 3 nesting levels.
…ildcard-import Wildcard imports make it harder to understand what is imported from where. Removing it is also a defensive programming act, lowering the probability of collisions due to future new imports or objects.
The methods get_string and from_string of the class MPFile had 14 branches each. Pylint recommends not to have more than 12 to reduce complexity, ease understanding and testing. I extracted from each method a small local method to reduce the number of branches and increase structuring.
Method run had 82 statements while pylint suggest not to have more than 50. I extracted some small local methods, adding structure and making the method shorter. Please note that the end of the method was not reachable (and still so) due to a break, which might be a bug. I added a TODO there to mark that.
The method add_structure of class MPFileCore had 17 branches while pylint recommends to have no more than 12. I extracted a small method _build_structure that builds the structure from the source while using many branches to handle the source possible types.
…sion.py too-many-statements Function run had 117 statements while pylint recommends not to have more than 50. The function was actually structured since it had different logics for different crystal structures. I extracted small unrelated functions for these logics.
The make function had 74 statement while pylint recommends to have at most 50. I extracted small methods handling parts of the function.
…on-caught Catching Exception instead of specific exceptions might hide unexpected ones. Function format_cell caught Exception. isnan does not raise exceptions https://docs.python.org/3/library/math.html#math.isnan str might lead to UnicodeEncodeError https://stackoverflow.com/questions/33556465/is-there-any-object-can-make-str-function-throw-error-or-exception-in-python
…caught Method to_backgrid_dict of class table catches Exception. Catching Exception instead of specific exceptions might hide unexpected ones. I changed to catching AttributeError, as in the StackOverflow post in the comment above.
Function get_specs had 19 branches while pylint suggest not to have more than 12. I extracted method for populating the data structures and handling the different method names.
…ission Method has_read_permission of class SwaggerView had 20 branches while pylint recommends to have no more than 12. I extracted method to reduce that, making the code more structured. This fixed also the too-many-return-statements and too-many-statements alerts in the same method.
|
Thanks! The most important piece of the code base to focus on would be the |
|
Thanks! This PR can be closed then, right? And the PR #1857 can be rebased on the main branch to exclude the update on the requirements files. |
|
Yes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Major changes:
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.
Todos
In mpcontribs-portal\mpcontribs\users\redox_thermo_csp\pre_submission.py I modified the method run since it had too-many-statments alerts.
Please note that the end of the method was not reachable (and still so) due to a break, which might be a bug.
I added a TODO there to mark that.
There were broad-exception-caught alerts in some files.
Such handling (e.g., catching Exception) might hide bugs due to unexpected exception.
Therefore it is recommended to catch sepcific exceptions.
I could not understand whare are the expected exceptions in few of the cases.
Therefore, I did not fix these alerts yet.
Can you help me with that?
mpcontribs-portal\mpcontribs\portal\views.py
Line 490 in make_download catches Exception
mpcontribs-ingester\mpcontribs\ingester\webui.py
Line 184 in view catches Exception
Line 242 in contribute catches Exception
mpcontribs-serverless\make_download\app.py
Line 60 in lambda_handler catches Exception
mpcontribs-client\mpcontribs\client_init_.py
Line 371 method display in class Table
Checklist
ruff.mypy.duecredit@due.dcitedecorators to reference relevant papers by DOI (example)Tip: Install
pre-commithooks to auto-check types and linting before every commit: