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

Fixing some linter issues #590

Merged
merged 1 commit into from
Jul 5, 2023
Merged

Conversation

erikbosch
Copy link
Contributor

I tried running mypy, pylint and flake8.
Nothing alarming found but many false positives,
so we are not mature to enable them in CI.
This addresses some low hanging fruits.

I tried running mypy, pylint and flake8.
Nothing alarming found but many false positives,
so we are not mature to enable them in CI.
This addresses some low hanging fruits.
@erikbosch erikbosch marked this pull request as ready for review June 30, 2023 13:09
Copy link
Contributor

@lukasmittag lukasmittag left a comment

Choose a reason for hiding this comment

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

What I did is:

  • Ran against databroker without authorization enabled (set, get, subscribe)
  • Ran against databroker with authorization enabled (set, get, subscribe, authorize)
    Worked

@@ -1,3 +1,15 @@
# /********************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought in this files we do not need a header @SebastianSchildt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the license as otherwise our own spdx license checker complains. As of today we require a license in all *.py files changed to make the checker happy. This could off course be changed, for example by explicitly excluding files called setup.py. But is it worth it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, indeed. Yeah dont think it is worth it then.

if token_or_tokenfile is None:
token_or_tokenfile = self.token_or_tokenfile
if os.path.isfile(token_or_tokenfile):
token_or_tokenfile = pathlib.Path(token_or_tokenfile)
token = token_or_tokenfile.expanduser().read_text(encoding='utf-8').rstrip('\n')
token_or_tokenfile_path = pathlib.Path(token_or_tokenfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this renaming here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is mypy that does not like when you change type of a variable. token_or_tokenfile was first a str and then we did an assignment changing it to Path. Now instead we use a different variable name.

error: Incompatible types in assignment (expression has type "Path", variable has type "str") [assignment]

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I see :)

@erikbosch erikbosch merged commit 09ca9b1 into eclipse:master Jul 5, 2023
6 checks passed
@erikbosch erikbosch deleted the erik_lint branch July 5, 2023 08:03
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