search

mllam / neural-lam

Neural Weather Prediction for Limited Area Modeling
MIT License
64 stars 25 forks source link

pre-commit for linting and formatting #6

Closed sadamov closed 5 months ago

sadamov commented 5 months ago

Added pre-commit support for linting and formatting. Applied new rules to all files in repo. Tested functionality on meps_example successfully (unchanged) I suggest to squash commits ;)

@joeloskarsson feel free to change rules to your asthetic preferences

leifdenby commented 5 months ago

@sadamov this looks great to me! Thanks for doing this, it has been on my todo list for a long time. Also, it's great you've set it to run with cicd too (running only on pushes and pull-requests to main seems sensible to me).

I've just noticed that you've added a pyproject.toml file with this pull-request. I realise you added that to configure the pre-commit linting tools, but having both requirements.txt and pyproject.toml isn't a great idea (since both are used for defining the project dependencies) also that file includes a version of the package :) Maybe we can have the versioning etc discussion before this pull-request is merged?

joeloskarsson commented 5 months ago

Regarding pyproject.toml Eventually we will probably want to build a package and then move all dependencies into pyproject.toml. For now maybe we can keep only the tool-configurations in pyproject.toml, but leave dependencies as is (specified in README and requirements.txt). I don't think just the existence of both files causing any inteference.

Versioning If we keep only the [tool....] entries then there would be no versioning included in pyproject.toml either. Anyhow we should be able to merge versioning or this PR in any order, just keeping in mind that the changes in this PR should be in the changelog.

@sadamov Can I have permission to push to the branch, so I can tweak the tool options? Think that might be easier than commenting and having you change them.

sadamov commented 5 months ago

Only using the [tools] in pyproject.toml for now seems like a sensible choice to me.

joeloskarsson commented 5 months ago

I added also pylint now. It is a bit slow to run, but I think it offers a lot of useful linting. It is also what I have used when developing most of the code, so its principles align fairly well with the style of the codebase. That said, there are a number of files where I have not been very strict with following this style, but I want to fix that now. So at the moment there's a list of messages from pylint, but I intend to make the changes to fix these.

joeloskarsson commented 5 months ago

I'm happy with this now. @sadamov could you

  1. Check that nothing looks strange in the changes I made to the configs. Maybe test again on your side that there are no functionality changes.
  2. Add a short note to the readme about how to use the pre-commit checks when working with the code?

Then we should be good to merge.

sadamov commented 5 months ago

I tested the code again, and everything works as expected. Thanks for integrating pylint as well! I think with that we are ready to merge @joeloskarsson