Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: add pre-commits #6

Merged
merged 6 commits into from
Apr 22, 2023

Conversation

12rambau
Copy link
Contributor

I don't know if you are aware of pre-commits, the idea is to run the linting, style rules on every git commit call.

I included:

  • black the famous Python linter:
  • ruff: a super fast implementation of all PEP rules:
  • nbstripout: remove all outputs from notebook to simplify their versionning
  • prettier, for anything that is not a .py file
  • doc8, like ruff but for the soon to come Sphinx documentation
  • licenceChek (not yet active) to check the compatibility of your license with the one from the dependencies

I included some ruff rules about docstring style and feel free to change it if your prefer numpy-style

I deactivated the docstring check but that would be nice to keep it in mind while coding. Cost nothing to do it upfront and save hours when debugging is needed

These hooks are also tested in a rudimentary Github Action to ensure they are respected in PR (when people only use the Github interface for example)

@12rambau
Copy link
Contributor Author

It seems that --dev is now deprecated in poetry and "dev" dependencies should be treated as any other group. I removed IPython from it (as it needs to be in the normal deps) and created the dev group.

@santiagobasulto
Copy link
Owner

This looks great! The only thing I'd change is removing nbstripout, as the output for Demo.ipynb also serves as documentation. See how many people opened it:
image

If we add ipython as a regular dependency, I think we should make sure to include the minimum possible required version. For example, ipyparallel uses >=4. I'm not 100% sure of iPython's Magic class API, which is the minimum version we can use that includes the Magic class API and the display methods we need.

@12rambau
Copy link
Contributor Author

12rambau commented Apr 17, 2023

This looks great! The only thing I'd change is removing nbstripout, as the output for Demo.ipynb also serves as documentation:

for the demo.ipynb I would like to include it in the documentation using mystnb (which execute the cells at build time) and add 2 links to open it in either binder or colab.

Easy to maintain for us (no output in the file), easy to reuse for readers. I have an example in one of my other libs (it's quantum computing so very unrelated): https://pyqutree.readthedocs.io/en/latest/examples/demo.html

On top of that instead of a static picture, a GIF of you interacting with the commands is easier to get as in commitizen: https://commitizen-tools.github.io/commitizen/

what do you think ?

@12rambau
Copy link
Contributor Author

If we add ipython as a regular dependency, I think we should make sure to include the minimum possible required version.

I think version number is something that will be easier to pin when the test and actions will be up and running be as smooth as you think and I'll try to create environment check asap

@santiagobasulto
Copy link
Owner

Alright, let's give it a test! I'm gonna merge this one so I can finish pushing a few tests and design changes that I have pending.

@santiagobasulto santiagobasulto merged commit 4d913f3 into santiagobasulto:master Apr 22, 2023
@12rambau 12rambau deleted the pre-commit branch April 22, 2023 13:52
@santiagobasulto
Copy link
Owner

@12rambau I commented out nbstripout for now so I can leave Demo.ipynb as documentation. But I like the idea of including mystnb, so I'll focus on documentation in the next few days.

@12rambau
Copy link
Contributor Author

no problem, I can update #8 and #7 to have a working framework. In the meantime could you create a project on ReadThedocs website and enable build on PR so that I test the result automatically (Only the owner of the repo can do that) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants