ECSHackWeek / impedance.py

A Python package for working with electrochemical impedance data
MIT License
221 stars 101 forks source link

Add pre-commit config #228

Closed MattiasMTS closed 1 year ago

MattiasMTS commented 2 years ago

What has changed:

Example of pre-commit for the flake8 checker:

  1. Let's say I am making some changes in the impedance.preprocessing.py file: image
  2. I git add and commit this feature with a nice and concise commit message, you will then see the following: image

    I can see that the linting fails on flake8 with the error messages given there. Note also that the changes I made didn't get committed. I would have to resolve these before proceeding (you can skip this with --no-verify command but we keep that secret)

    image

    And now it allows me to git commit this new change.

MattiasMTS commented 2 years ago

Just saw that the CI is failing due to splitting the requirements.txt file. This can be changed depending on #227 . For now, the reviewer can focus on the other changes.

I noticed that the tests/ folder lives in the impedance/ folder. Usually, a good practice is to have the tests folder outside the actual module folder, since it is focusing on "reflecting" the structure of the module. Furthermore, when you build and ship this package - you then install unnecessary dependency on the user. For example, this package now requires the user to install pytest since the tests folder uses it. Moving the tests folder outside and mirroring the structure of impedance allows you to easier decouple some dependencies more. Any thoughts on this @mdmurbach ?

mdmurbach commented 2 years ago

Just saw that the CI is failing due to splitting the requirements.txt file. This can be changed depending on #227 . For now, the reviewer can focus on the other changes.

I noticed that the tests/ folder lives in the impedance/ folder. Usually, a good practice is to have the tests folder outside the actual module folder, since it is focusing on "reflecting" the structure of the module. Furthermore, when you build and ship this package - you then install unnecessary dependency on the user. For example, this package now requires the user to install pytest since the tests folder uses it. Moving the tests folder outside and mirroring the structure of impedance allows you to easier decouple some dependencies more. Any thoughts on this @mdmurbach ?

Yeah, definitely agree (and know now) that putting the tests/ directory at the top level of the repo makes much more sense. Feel free to make that change in this PR or I can follow up after and make that change :)

PS thanks for all of your help recently getting the project from it's early hackweek state to a more robust maintainable package, definitely appreciate it and it's overdue 👍

mdmurbach commented 2 years ago

Going to hold off merging this one in since I think it makes sense to leverage poetry for the requirements