Skip to content
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.

Sign up for GitHub

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

Jump to bottom

random cleanups / fixes #1879

Merged
merged 9 commits into from
Jan 9, 2024
Merged

random cleanups / fixes #1879

merged 9 commits into from
Jan 9, 2024

Conversation

ThomasWaldmann
Copy link
Collaborator

@ThomasWaldmann ThomasWaldmann commented Jan 8, 2024 •
edited
Loading

most stuff was found by pycharm code checker.

commit 48af51f should fix some potential malfunction, that function is called without params rather often, thus site=None -> bug.

@ThomasWaldmann
fix PEP8 E721
2eb0641 
do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
@ThomasWaldmann
remove redundant parentheses
e60a437
@ThomasWaldmann
fix SiteWorker.run for empty job queue
19d1ba4 
local variable job is not assigned if queue was empty
when calling .run(), but it is used in exception handler.
@ThomasWaldmann
remove unreachable code in parse_diff_lines
910f324
@ThomasWaldmann
bug fix for unreachable code in is_worker_running
48af51f 
the code intended to check if *any* worker is running for
any site was *unreachable*.

this caused false negative results for site=None.
@ThomasWaldmann
check_failed_response: remove outdated part of docstring
265414b
@ThomasWaldmann
pull request template: fix relative path to LICENSE.txt
01e55b6
@ThomasWaldmann
fix typos
3662e10
@ThomasWaldmann
use logger.warning, .warn is deprecated
3a84e39
Hofer-Julian
Hofer-Julian approved these changes Jan 8, 2024
View reviewed changes
Copy link
Collaborator

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
The pycharm code checker seems to be quite useful.

@ThomasWaldmann
Copy link
Collaborator Author

@Hofer-Julian It's a bit of a mixed experience:

  • sometimes it finds something useful (like here)
  • sometimes it has false positives (valid code, but the checker didn't get it)
  • lots of useless stuff (like unknown words)
  • the sheer volume of warnings can be rather high (vorta: > 50000)

Luckily, the result is somehow structured, so one does not need to look at all of the warnings.

@Hofer-Julian
Copy link
Collaborator

@Hofer-Julian It's a bit of a mixed experience:

* sometimes it finds something useful (like here)

* sometimes it has false positives (valid code, but the checker didn't get it)

* lots of useless stuff (like unknown words)

* the sheer volume of warnings can be rather high (vorta: > 50000)

Luckily, the result is somehow structured, so one does not need to look at all of the warnings.

Interesting.

I guess, we prefer tools anyway that have a CLI and can be easily run on the CI like typos and ruff.

@m3nu m3nu merged commit 675010e into borgbase:master Jan 9, 2024
@ThomasWaldmann ThomasWaldmann deleted the cleanups branch January 9, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m3nu m3nu m3nu approved these changes

@Hofer-Julian Hofer-Julian Hofer-Julian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ThomasWaldmann @Hofer-Julian @m3nu