ManderaGeneral/generalimport

Linting

ZanSara opened this issue · 2 comments

ZanSara commented

As discussed in #18, we can consider adding some linting and code formatting facilities to this project. I opened the issue here not to make noise on generalpackager, but feel free to move it there if you deem it suitable.

I'm basing this recommendation on what we have in Haystack and related projects, like Canals. Canals is way simpler and smaller than Haystack, so you can take a look at it to make yourself an idea of what I have in mind 🙂

  • Code formatting: black
  • Type checking: mypy
  • Linting: pylint

These tools need close to no configuration, but all the config would go in the pyproject.toml file. So I recommend introducing that one first (ManderaGeneral/generalpackager#78).

They could run in the CI and fail if they hit any error. This is an example workflow from Canals running all these three (plus the unit tests): https://github.com/deepset-ai/canals/blob/main/.github/workflows/tests.yml

We also add these same tools in the pre-commit hooks like this: https://github.com/deepset-ai/canals/blob/main/.pre-commit-config.yaml In this way, the CI almost never fails and it can be used to make sure contributors installed the pre-commit hooks.

Mandera commented

black and pylint sounds great for all repos! Not sure about mypy. I'm open to making it an option for each repo and then we'll enable it with generalimport to begin with. What are the pros with type checking? I do love type hinting but I don't understand enforcing it

ZanSara commented

I found mypy to be quite helpful in general. It does not force you to type everything, it can normally guess the types correctly regardless, and when it raises issues it's always catching some small bug. So it has been a widely net positive for me to use it.

Interestingly for me pylint is less useful because, although it helps a lot, it also has an opinionated view and sometimes it conflicts with mine 😄

Of course it's up to you. I run it like mypy generalimport/ --exclude='test/' on the codebase and I got just three errors, so it should be easy to integrate. But it's not a dealbreaker.