ci: add flake8 and fix warnings #326
Conversation
| @@ -479,7 +479,7 @@ class TexasEastern(_Builder): | |||
|
|
|||
| name = "Texas_Eastern" | |||
|
|
|||
| def __init__(self): | |||
| def __init__(self, ssh_client): | |||
There was a problem hiding this comment.
This class is never instantiated. That is why it is not maintained
There was a problem hiding this comment.
We could just use pass after the definition.
There was a problem hiding this comment.
If we never use it, and we never foresee using it, should we remove it?
There was a problem hiding this comment.
Yeah we should remove if there aren't plans for it (I wasn't sure what the status was). I suppose you would be someone to ask about that, so I will go ahead, we can always put it back later since it's still in git.
There was a problem hiding this comment.
Well... we may want to keep it in (with the fixes) for other people to use. Coupling between Texas and Eastern isn't strong currently, but someone might want to study a scenario where the coupling improves between these two, while Western remains relatively isolated, in which case they would want to use this.
| @@ -490,7 +490,7 @@ class EasternWestern(_Builder): | |||
|
|
|||
| name = "Eastern_Western" | |||
|
|
|||
| def __init__(self): | |||
| def __init__(self, ssh_client): | |||
|
It seems that we could check docstring through Or we could use Any thoughts? |
| @@ -461,5 +461,5 @@ def test_grid_eq_failure_storage(base_texas): | |||
| def test_that_fields_are_not_modified_when_loading_another_grid(): | |||
| western_grid = Grid(["Western"]) | |||
| western_plant_original_shape = western_grid.plant.shape | |||
| eastern_grid = Grid(["Eastern"]) | |||
| eastern_grid = Grid(["Eastern"]) # noqa | |||
There was a problem hiding this comment.
What does this comment mean?
For context, this test was to demonstrate a bug that has since been fixed. Loading an Eastern grid modified existing Westerns. I believe the test would achieve the same thing even if the Eastern grid is not stored in a variable.
There was a problem hiding this comment.
There was a problem hiding this comment.
It causes flake8 to ignore that line, not sure why I didn't just remove the variable name but will do that.
| @@ -30,8 +30,8 @@ def get_table(self, filename, path_on_server): | |||
| try: | |||
| table = self._get_from_server(path_on_server) | |||
| table.to_csv(local_path, index=False) | |||
| except: | |||
| print(f"Failed to download {filename} list from server.") | |||
| except Exception as e: | |||
There was a problem hiding this comment.
Is the idea that any general except must at least print the exception caught?
There was a problem hiding this comment.
The error message will be cluttered
There was a problem hiding this comment.
The warning is about having an empty except: block, but figured we might as well print the error.
There was a problem hiding this comment.
Yeah I didn't see a lot of value in printing this one, so updated it to ignore the flake8 warning.
There was a problem hiding this comment.
I think some of the reason for the generality of the exception catching is that it's hard to catch paramiko exceptions, something about them being part of a separate thread, so it's hard to know what we'll get if _get_from_server doesn't complete successfully. But this is second-hand info from @rouille.
|
Yeah we could add plugins for flake8 to check docstrings and other things, it's very extensible, just wanted to keep this simple to start. |
|
Removing all those unused imports is beautiful. |
Purpose
Add flake8 linting which can catch typos in variable names, unused imports, undefined variables, etc. In the process, I found 2 actual bugs where there is an undefined local variable (see
create.py)What the code is doing
This adds flake8 to the default tox envlist, and adds a new job to the github lint workflow. To make it pass, I fixed warnings, except for a few that made more sense to ignore. List of error codes is here.
Testing
CI
Where to look
tox.ini - how flake8 is run and configured, everything else is code fixes
Time estimate
10 min