-
Notifications
You must be signed in to change notification settings - Fork 51
Conversation
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.
There was a problem hiding this 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 @@ | |||
# /******************************************************************************** |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I see :)
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.