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

CI: Test out nbqa for linting notebooks #63

Merged
merged 8 commits into from
Jun 14, 2022

Conversation

MridulS
Copy link
Member

@MridulS MridulS commented Mar 9, 2022

We can use https://github.com/nbQA-dev/nbQA to lint and format our notebooks, it can run many linters like black, isort, pyupgrade and also format the markdown cells in the notebook.

#45

@MridulS
Copy link
Member Author

MridulS commented May 17, 2022

@rossbar can you have a look here? This PR adds a linting step to our CI with pre-commit and nbQA.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @MridulS , I like this idea and the setup is pretty straightforward. It would be super awesome if the tooling around notebooks (nbQA and nbval come to mind) supported the text-based markdown format, then we could pull this off without all the jupytext conversions (and actually use pre-commit in the repo rather than only in CI!).

That's beyond the scope here though, this is definitely an improvement IMO!

One thought re: organization: would it be worth cleaning up the history a bit and squash everything down to two commits: one where the CI infrastructure is added, and another where the linting is applied to the existing notebooks? That way, we can do the .git-blame-ignore-revs thing for the commit with the content changes. It's probably not super important for the nx-guides history but it might be nice to do if it's not too much trouble. Other than that, LGTM! I'll let you make the decision re: history munging and please feel free to self-merge or ping me again!

@rossbar
Copy link
Contributor

rossbar commented Jun 14, 2022

One thought re: organization: would it be worth cleaning up the history a bit and squash everything down to two commits: one where the CI infrastructure is added, and another where the linting is applied to the existing notebooks?

Meh let's not let this be a blocker. In this goes, thanks for setting this up @MridulS !

@rossbar rossbar merged commit 73cf5d8 into networkx:main Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants