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

[pre-commit] added clean notebook output hook #639

Merged
merged 6 commits into from
Nov 10, 2021

Conversation

marscher
Copy link
Contributor

@marscher marscher commented Nov 9, 2021

Changes

Added clean notebook output hook. Tested locally, works like a charm.

There is an additional argument to remove kernel metadata (e.g. the constant switching of the Python version, depending on the devs local version). But this also removed the name of the env "weldx". So I'm not sure if this would break something. That's why I just disabled it.

@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #639 (9a0617b) into master (299d199) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #639   +/-   ##
=======================================
  Coverage   94.49%   94.49%           
=======================================
  Files          93       93           
  Lines        6011     6011           
=======================================
  Hits         5680     5680           
  Misses        331      331           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 299d199...9a0617b. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Nov 9, 2021

Unit Test Results

       1 files  ±0         1 suites  ±0   2m 1s ⏱️ +21s
1 930 tests ±0  1 930 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 9a0617b. ± Comparison against base commit 299d199.

♻️ This comment has been updated with latest results.

@marscher marscher requested a review from CagtayFabry November 9, 2021 10:43
@CagtayFabry
Copy link
Member

nice find, this would replace our check-notebooks job

we should certainly try the side effects of the kernel info removal.
For the users this would probably prompt a kernel selection on notebook load (which I think is fine), in our pytest and sphinx runs we might have to set the kernel explicitly (which should be doable, for sphinx we already do/did that)

@marscher
Copy link
Contributor Author

marscher commented Nov 9, 2021

I guess this would simply our CI pipeline a bit, since we need to set the correct environment (or just override the existing kernel metadata). I can open up a follow up PR with removed kernel metadata to see what happens.

Copy link
Member

@CagtayFabry CagtayFabry left a comment

Choose a reason for hiding this comment

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

nice, thanks for the cleanup

looks like the python version doesn't get cleaned after all but still nice to have a good way to format

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CagtayFabry CagtayFabry requested a review from vhirtham November 10, 2021 08:12
@marscher marscher merged commit 44e3475 into BAMWelDX:master Nov 10, 2021
@marscher marscher deleted the precommit-clean-nb-output branch November 10, 2021 09:23
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.

2 participants