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

Nicer code review experience with jupytext and black #62

Merged
merged 8 commits into from
Nov 27, 2018
Merged

Conversation

weiji14
Copy link
Owner

@weiji14 weiji14 commented Nov 26, 2018

Provide a better experience for people reviewing the code by setting some standards.

Jupyter notebooks are nice but large git diffs can be hard to see, and some standard Python code review tools don't work nicely (if at all) with notebooks. Jupytext combines the beauty of Jupyter notebooks with standard plain python files, allowing for more flexibility in development. See also ReviewNB for another tool in the space of making .ipynb diffs easier to visualize.

WIth jupytext in place, next step is to lint the plain python (.py) scripts and have the changes synced to our jupyter notebooks (.ipynb). In other words, follow the PEP 8 style guide! Ideally this should be automatically enforced, which sounds painful (and it can be at first), but it will provide consistency moving forward. Black is one such (opinionated) code formatter tool I've been slowly getting used to, and it is time to bring it into the project.

Part of #47.

References:

TODO:

Allows notebooks to be stored as plain python scripts too! See https://github.com/mwouts/jupytext. Choosing the py:percent format https://github.com/mwouts/jupytext#the-percent-format that works with Hydrogen in the Atom editor and other IDEs. This py:percent configuration is set in the .jupyter/jupyter_notebook_config.py file, which itself is referenced via a JUPYTER_CONFIG_DIR environmental variable set in the .env file.

Once we start pairing .ipynb and .py files, this jupytext setup will allow for nicer git diffs, and hopefully ease development workflows. Will look at introducing linters, proper code coverage possibly, and who knows what else!
@weiji14 weiji14 added enhancement ✨ New feature or request python Pull requests that update Python code labels Nov 26, 2018
@weiji14 weiji14 added this to the v0.5.0 milestone Nov 26, 2018
@weiji14 weiji14 self-assigned this Nov 26, 2018
Part of #62. Initial pairing of the data_prep.ipynb and srgan_train.ipynb jupyter notebooks with their equivalent .py files in the root directory of the repository. Pending linting with black. Also renamed the features/steps/data_prep.py file to test_data_prep.py to avoid confusion.
The uncompromising Python code formatter. See https://github.com/ambv/black
Changing single quotes to double quotes, enforce max line length of 88 for code, strip whitespace, etc. See https://black.readthedocs.io/en/stable/the_black_code_style.html

Changes made by 1) adding "formats": "ipynb,py:percent" to the jupyter notebook's metadata. 2) Saving the .py files using python-black v0.6.0 inside the Atom editor https://github.com/mikehoyio/atom-python-black, which automatically triggers the linting in the .py and .ipynb script, and 3) Using git add --patch *.ipynb to only stage the changed code and not the outputs of the cell (which disappears...), with some fine tuning in gitkraken.
Only a few newlines added to the actual code with black linter. This notebook/script pairing almost goes without saying. But I do wonder if I can simplify the _load_ipynb_modules function in environment.py now that we have paired notebooks.
From https://shields.io and https://github.com/ambv/black#show-your-style respectively. Also remove the libaries.io badge which keeps showing up as invalid.
Repository owner deleted a comment from stickler-ci bot Nov 26, 2018
Repository owner deleted a comment from stickler-ci bot Nov 26, 2018
Also disable automatic fixing. Check to see if pyproject.toml config file actually needed.
@weiji14 weiji14 changed the title WIP Nicer code review experience with jupytext and black Nicer code review experience with jupytext and black Nov 27, 2018
@weiji14 weiji14 merged commit 4a63b69 into master Nov 27, 2018
@weiji14 weiji14 deleted the jupytext branch November 27, 2018 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants