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.

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

Minor changes #811

Merged
merged 8 commits into from
Jun 5, 2024
Merged

Minor changes #811

merged 8 commits into from
Jun 5, 2024

Conversation

almet
Copy link
Member

@almet almet commented May 22, 2024

A small pull request with changes I did while reading the current codebase.

I've added some comments directly on Github when it made sense.

dangerzone/gui/logic.py Outdated Show resolved Hide resolved
@almet almet force-pushed the almet/small-improvements branch 2 times, most recently from 37f45a5 to 1b8bc43 Compare May 22, 2024 15:15
@almet almet marked this pull request as draft May 22, 2024 16:13
@almet
Copy link
Member Author

almet commented May 22, 2024

I converted this PR to a draft for now, until I have the tests running properly on my machine, so I can replicate what the CI is telling me :-)

@almet almet force-pushed the almet/small-improvements branch from 1b8bc43 to 756fda8 Compare May 23, 2024 09:45
@almet
Copy link
Member Author

almet commented May 23, 2024

I've put the changes for pytest fixtures in a separate PR: #813

@almet
Copy link
Member Author

almet commented May 23, 2024

Current linting failures are due to a bug on mypy, which will mark code as unreachable if behind a boolean assert, like this:

assert something is True
assert somethingelse # <-- this is considered unreachable

I'm considering removing the --warn-unreachable flag for the tests.

@almet
Copy link
Member Author

almet commented May 23, 2024

Tests are failing due the latest Alpine image. This should be fixed with #812.

@apyrgio I'm curious about your feedback on disabling the mypy --warn-unreachable flag for the tests. Maybe there is a better way :-)

@almet almet force-pushed the almet/small-improvements branch from ab68727 to 5e4f46f Compare May 23, 2024 12:26
@almet almet marked this pull request as ready for review May 23, 2024 12:26
@apyrgio
Copy link
Contributor

apyrgio commented May 23, 2024

@apyrgio I'm curious about your feedback on disabling the mypy --warn-unreachable flag for the tests. Maybe there is a better way :-)

I'd be more inclined to use # type: ignore [unreachable] in affected lines, since there may be an actual case where something is unreachable in our tests, and we want t the linter to flag it.

@almet almet force-pushed the almet/small-improvements branch from 5e4f46f to efb98cb Compare May 23, 2024 12:39
@almet
Copy link
Member Author

almet commented May 23, 2024

I'd be more inclined to use # type: ignore [unreachable] in affected lines, since there may be an actual case where something is unreachable in our tests, and we want t the linter to flag it.

That's indeed a better solution :-) I've updated the branch accordingly, and force-pushed.

dangerzone/__init__.py Outdated Show resolved Hide resolved
dangerzone/gui/updater.py Outdated Show resolved Hide resolved
dangerzone/util.py Show resolved Hide resolved
@almet almet force-pushed the almet/small-improvements branch from 6000913 to f972917 Compare May 27, 2024 11:59
@apyrgio apyrgio added this to the 0.7.0 milestone Jun 3, 2024
Copy link
Contributor

@apyrgio apyrgio left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, they look good to me. I think once you rebase this PR on top of the main branch, and all the tests pass, you can merge it.

@almet almet force-pushed the almet/small-improvements branch from f972917 to 8d6cc45 Compare June 4, 2024 12:59
@almet
Copy link
Member Author

almet commented Jun 4, 2024

Just rebased this branch on top of latest main

@almet almet force-pushed the almet/small-improvements branch 2 times, most recently from 6526560 to a49e4e9 Compare June 5, 2024 11:32
almet added 5 commits June 5, 2024 14:19
As detected by [ruff](https://github.com/astral-sh/ruff)

Related to #254, although it doesn't provide the command to lint the
codebase itself.
> f-strings are a convenient way to format strings, but they are not
> necessary if there are no placeholder expressions to format. In this
> case, a regular string should be used instead, as an f-string without
> placeholders can be confusing for readers, who may expect such a
> placeholder to be present.
>
> — [ruff docs](https://docs.astral.sh/ruff/rules/f-string-missing-placeholders/)
This commit removes code that's not being used, it can be exceptions
with the `as e` where the exception itself is not used, the same with
`with` statements, and some other parts where there were duplicated
code.
A few minor changes about when to use `==` and when to use `is`.
Basically, this uses `is` for booleans, and `==` for other values.

With a few other changes about coding style which was enforced by
`ruff`.
Bare excepts will catch keyboard-exit exceptions, system-exit etc. which
is probably not what we want.
@almet almet force-pushed the almet/small-improvements branch from a49e4e9 to d9d9ab9 Compare June 5, 2024 12:21
@almet almet merged commit d9d9ab9 into freedomofpress:main Jun 5, 2024
11 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants