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

docs: adopt nbfmt.py formatting, enforced in CI #3455

Merged
merged 3 commits into from Mar 31, 2020
Merged

Conversation

wchargin
Copy link
Contributor

Summary:
Notebook formatting is a frequent source of friction in pull requests,
due to spurious diffs and unintended changes. @lamberta’s nbfmt.py is
an opinionated autoformatter that should resolve these issues entirely.

Test Plan:
A test run of the CI change fails before the reformat and passes after:

Both the failure and passing cases have nice output.

wchargin-branch: docs-nbfmt

This is expected to fail as written.
It was always a bit brittle, and is now conflicting with `nbfmt.py`.
This test has been valuable maybe once, whereas `nbfmt.py` would have
been valuable on nearly every docs change. `nbfmt.py` wins.
Generated with:

```
$ git ls-files -z '*.ipynb' | xargs -0 python3 ./nbfmt.py --ignore_warn
Notebook: docs/get_started.ipynb
Notebook: docs/graphs.ipynb
Notebook: docs/hyperparameter_tuning_with_hparams.ipynb
Notebook: docs/image_summaries.ipynb
Notebook: docs/migrate.ipynb
  Removed the existing output cells.
Notebook: docs/scalars_and_keras.ipynb
Notebook: docs/tbdev_getting_started.ipynb
Notebook: docs/tensorboard_in_notebooks.ipynb
Notebook: docs/tensorboard_profiling_keras.ipynb
Notebook: tensorboard/plugins/mesh/Mesh_Plugin_Tensorboard.ipynb
  Missing license: #@title Licensed under the Apache License
  Missing copyright: Copyright 20[1-9][0-9] The TensorFlow\s.*?\s?Authors
```

Test Plan:
Running `git ls-files -z '*.ipynb' | xargs -0 ./nbfmt.py --test` passes.
@review-notebook-app

This comment has been minimized.

pip install absl-py
chmod +x ./nbfmt.py
- name: 'Check Colab notebooks for formatting'
run: git ls-files -z '*.ipynb' | xargs -0 ./nbfmt.py --test
Copy link
Collaborator

Choose a reason for hiding this comment

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

like the preventative work here to avoid quoted fns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I generally try to be funnynames-safe when it’s not too
inconvenient, and in most cases it’s easy enough.

@wchargin wchargin merged commit 795ab6b into master Mar 31, 2020
@wchargin wchargin deleted the wchargin-docs-nbfmt branch March 31, 2020 18:49
bileschi pushed a commit to bileschi/tensorboard that referenced this pull request Apr 15, 2020
Summary:
Notebook formatting is a frequent source of friction in pull requests,
due to spurious diffs and unintended changes. @lamberta’s `nbfmt.py` is
an opinionated autoformatter that should resolve these issues entirely.

Test Plan:
A test run of the CI change fails before the reformat and passes after:

  - <https://github.com/tensorflow/tensorboard/runs/549352035>
  - <https://github.com/tensorflow/tensorboard/runs/549365513>

Both the failure and passing cases have nice output.

wchargin-branch: docs-nbfmt
bileschi pushed a commit that referenced this pull request Apr 15, 2020
Summary:
Notebook formatting is a frequent source of friction in pull requests,
due to spurious diffs and unintended changes. @lamberta’s `nbfmt.py` is
an opinionated autoformatter that should resolve these issues entirely.

Test Plan:
A test run of the CI change fails before the reformat and passes after:

  - <https://github.com/tensorflow/tensorboard/runs/549352035>
  - <https://github.com/tensorflow/tensorboard/runs/549365513>

Both the failure and passing cases have nice output.

wchargin-branch: docs-nbfmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants