CFE-4554: Added pyright, pyflakes, and flake8 to GH Actions #246
Conversation
olehermanse
force-pushed
the
pyright
branch
8 times, most recently
from
e8680ed to
ca25e28
Compare
olehermanse
changed the title
Ticket: CFE-4554 Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
Ticket: CFE-4554 Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
Ticket: CFE-4554 Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
Ticket: CFE-4554 Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
Ticket: CFE-4554 Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
Ticket: CFE-4554 Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
Ticket: CFE-4554 Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
olehermanse
changed the title
Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>
|
|
||
|
|
||
| def read_json(path) -> OrderedDict: | ||
| def read_json(path) -> Union[OrderedDict, None]: |
There was a problem hiding this comment.
This seems weird, perhaps it would be better to not return None and instead use exceptions.
There was a problem hiding this comment.
In general, I think several suggested asserts should instead be handled exceptions. Some of them might be due to this.
There was a problem hiding this comment.
This seems weird, perhaps it would be better to not return
Noneand instead use exceptions.
In general, there's nothing weird or wrong about having a function which returns None on error. In some cases it might be nicer, in other using an exception can have benefits.
In general, I think several suggested asserts should instead be handled exceptions. Some of them might be due to this.
Yea, some of them should be, however making the assumptions explicit with asserts and enabling stricter enforcement for PRs going forward is still an improvement. Converting the codebase to try to exhaustively handle all error cases will be a bit bigger effort that is outside the scope of this PR.
There was a problem hiding this comment.
I think assert is not the right tool for the job of checking conditions like whether a file exists or not, I think it's rather for checking state that should never be wrong if there are no programming logic mistakes. They can even be disabled. I'm fine with allowing it for the moment in this PR, though. I was just concerned that pyright is suggesting adding such asserts.
There was a problem hiding this comment.
I think assert is not the right tool for the job of checking conditions like whether a file exists or not, I think it's rather for checking state that should never be wrong if there are no programming logic mistakes. They can even be disabled. I'm fine with allowing it for the moment in this PR, though. I was just concerned that pyright is suggesting adding such asserts.
Correct, it is not. asserts are a tool for highlighting programmer's mistakes and making assumptions explicit. That can include whether files exist or not. Currently the code assumes that a file exists in certain places, and it's better that this assumption is explicit rather than implicit.
jakub-nt
left a comment
jakub-nt
left a comment
There was a problem hiding this comment.
Can always adjust whatever in future PRs
2 participants
No description provided.