Add the ruff linter to the pre-commit check #214
Conversation
reverted lambda change
| "NPY", # numpy style | ||
| "PL", # pylint | ||
| # "PD", # pandas style # TODO enable later | ||
| # "C90", # code complexity # TODO enable later |
There was a problem hiding this comment.
these last two will require a reasonable number of (simple) changes, so I've left them off for now so this diff isn't too polluted.
There was a problem hiding this comment.
Nice idea, thanks. Looking forward to the next linting PR. :)
| _count_comm_group_vectorised, | ||
| _match_wildcards, | ||
| _process_comm_groups_vectorised, | ||
| commodity_map, |
There was a problem hiding this comment.
^ This was the original motivation - standard import sorting/formatting
| @@ -124,8 +123,8 @@ def parse_parameter_values_from_file( | |||
|
|
|||
|
|
|||
| def save_data_with_headers( | |||
| param_data_dict: Dict[str, Union[pd.DataFrame, List[str]]], | |||
| headers_data: Dict[str, List[str]], | |||
| param_data_dict: dict[str, pd.DataFrame | list[str]], | |||
There was a problem hiding this comment.
What python versions are you planning to support?
This was done with the py11 setting (to match CI), but I'll revert and re-run for py39 if that's the minimum target. (I think the | type hint only works from 3.10 onwards)
There was a problem hiding this comment.
We haven't had that discussion actually... How about py12?
There was a problem hiding this comment.
Works for me, type hints are a lot cleaner in 3.12 for instance.
I just thought someone mentioned a use-case for running with an older version is all.
There was a problem hiding this comment.
I think we use the match-case statement somewhere in transforms.py which is >= 3.11. I feel perhaps we should try to keep the min version as low as possible, just to make it easier for users? Not sure how much of a pain it is to install a specific Python version in Windows (in linux I use pyenv)
There was a problem hiding this comment.
I'd say it is easy! :-) But I am also fine with sticking to 3.11.
There was a problem hiding this comment.
Yeah I use pyenv on windows also, works very well.
Less savvy users can just download and run the official installers etc.
Ok, so we'll target >=3.11, cool.
| @@ -65,6 +65,7 @@ def run_gams_gdxdiff( | |||
| stdout=subprocess.PIPE, | |||
| stderr=subprocess.STDOUT, | |||
| text=True, | |||
| check=False, | |||
There was a problem hiding this comment.
|
I generally like linters, but I don't have that much experience using them with python. I've used ESLint with javascript. |
There was a problem hiding this comment.
Great idea, thanks! I've been thinking of adding a linter for a while, though I haven't used ruff myself, it looks neat (and fast!).
I still have an open branch somewhere to upgrade the version of pyright -- this should also lead to a bunch of pandas-related changes, perhaps ruff will help us fix many of those?
| "NPY", # numpy style | ||
| "PL", # pylint | ||
| # "PD", # pandas style # TODO enable later | ||
| # "C90", # code complexity # TODO enable later |
There was a problem hiding this comment.
Nice idea, thanks. Looking forward to the next linting PR. :)
| @@ -124,8 +123,8 @@ def parse_parameter_values_from_file( | |||
|
|
|||
|
|
|||
| def save_data_with_headers( | |||
| param_data_dict: Dict[str, Union[pd.DataFrame, List[str]]], | |||
| headers_data: Dict[str, List[str]], | |||
| param_data_dict: dict[str, pd.DataFrame | list[str]], | |||
There was a problem hiding this comment.
I think we use the match-case statement somewhere in transforms.py which is >= 3.11. I feel perhaps we should try to keep the min version as low as possible, just to make it easier for users? Not sure how much of a pain it is to install a specific Python version in Windows (in linux I use pyenv)
siddharth-krishna
changed the title
fixed loop lint errors
|
@SamRWest I guess this one can be merged already? Just so we don't have to resolve too many conflicts at a later stage... |
I've been finding the ruff linter really useful in some other projects, so thought I'd see if you guys were interested in adding it here.
My motivation was mainly to get the imports formatting standardised (which black doesn't do) for easier diffing/merging.
But it's got a bunch of other really useful checks, especially when working with pandas (though I've turned these off for now so as not to confuse the PR diff).
It's also found and fixed a bunch of style/safety things and I've flagged a couple of logic errors to be fixed also.
What do you guys think?