Removed unused imports from files #3906
Conversation
|
Modified commit message because this PR would not get rid of ALL unused imports so #3483 should remain open even if this PR lands. |
|
@cclauss I have removed unused imports till .\openlibrary\plugins\openlibrary\utils.py so should I add them here 2 at a time? or create separate PR for each of them? |
|
@cclauss Travis CI seems unhappy with batch 7/8/9/10 |
|
@cclauss can you help me with what all changes do I need to make. It says the line is too long but I actually removed a word so I am a little clueless here. |
|
Once you change a line, then that line must comply with strict |
openlibrary/catalog/marc/parse.py
Outdated
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| from openlibrary.catalog.marc.get_subjects import subjects_for_work | |||
| from openlibrary.catalog.marc.marc_base import BadMARC, NoTitle, MarcException | |||
| from openlibrary.catalog.utils import pick_first_date, tidy_isbn, flip_name, remove_trailing_dot, remove_trailing_number_dot | |||
| from openlibrary.catalog.utils import pick_first_date, tidy_isbn, remove_trailing_dot, remove_trailing_number_dot | |||
There was a problem hiding this comment.
| from openlibrary.catalog.utils import pick_first_date, tidy_isbn, remove_trailing_dot, remove_trailing_number_dot | |
| from openlibrary.catalog.utils import ( | |
| pick_first_date, remove_trailing_dot, remove_trailing_number_dot, tidy_isbn | |
| ) |
SaravgiYash
force-pushed
the
3483/refactor/removed-unused-imports
branch
from
3683202 to
4258733
Compare
|
@cclauss I have done the required changes and added some new commits. Status check is missing from initial 6 commits so did I do something wrong? |
|
I still have concerns about #3483 (comment) |
|
@cclauss so should I create a PR for each commit separately. But if the build is successful then there shouldn't be any problem?? |
For that, the integration tests would need to be running #3682 and probably added to significantly. Can you please create a subset of this PR that only removes Python standard library imports? That should be safer and once that lands, we can rebase this PR on master and land the non-standard library changes. |
|
@cclauss Is there a way I can automatically check for only python standard library unused imports, or do I have to do that manually? |
|
Manual I believe. Do you use git on the command-line? |
|
@Yashs911 I'd strongly suggest breaking this PR into a few smaller PRs so we can help you merge them much more quickly :) Some of these changes are very straight forward (like the unused standard libraries: Some of the Open Library specific imports may have strange side effects which needs to be tested (I am not sure I blindly trust the linter in these cases) |
|
Thank you @cclauss for investing time in this one <3 |
|
@mekarpeles Do I still need to break this PR into smaller PR or after #3909 this can be rebased as suggested by @cclauss . |
* removed unused imports 1 * batch2 * branch3 * batch4 * batch5 * batch6 * batch 7/8/9/10 * batch11 * batch12 * batch13 Co-authored-by: Christian Clauss <cclauss@me.com>
| @@ -32,7 +32,6 @@ | |||
| from infogami.utils.view import safeint | |||
|
|
|||
| # TODO: i18n should be moved to core or infogami | |||
| from openlibrary.i18n import gettext as _ | |||
There was a problem hiding this comment.
@cclauss This looks like it is supposed to be where the i18n _() helper method that translates all the UI strings is imported. Has this been tested to ensure i18n doesn't break?
Even if this is non-breaking, the comment in line 34 relates to the deleted line and doesn't make any sense without it.
There was a problem hiding this comment.
Thanks. _() is not called in this file and a line like 69 (_, host, _, _, _ = urlsplit(href)) is going to shadow its value. I will create PR #3966 to remove the comment.
Related to #3483
refactor removed unused imports will commit 2 files at a time
Technical
Testing
Screenshot
Stakeholders
@cclauss @imskr