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

Remove deepcopy #694

Merged
merged 6 commits into from Oct 18, 2022
Merged

Remove deepcopy #694

merged 6 commits into from Oct 18, 2022

Conversation

surdouski
Copy link
Contributor

Just found this one after updating something internal and hitting serialization issue again. See previous PR regarding deepcopy (they have no purpose).

Just found this one after updating something internal and hitting serialization issue again. See previous PR regarding deepcopy (they have no purpose).
@MSeal
Copy link
Member

MSeal commented Oct 7, 2022

There is a purpose to deepcopy, in the interface, but I agree removing it should be expected at this point. Very early on there was more manipulation of nbformat objects and some implicit expectations that the input wouldn't be modified. Mostly when scrapbook code was still part of papermill. Not as much of a concern now-a-days so happy to merge it.

@MSeal MSeal enabled auto-merge (squash) October 7, 2022 01:13
Engine input isolation removed, update test with deepcopy.
auto-merge was automatically disabled October 7, 2022 13:51

Head branch was pushed to by a user without write access

Remove test with this expectation.
@surdouski
Copy link
Contributor Author

I figured I could quickly update this in browser, I'll wait till I'm home later today to actually pull everything down and run the tests locally before committing anything else to the branch.

Used deepcopy where necessary when tests used the isolated nb
expectation in unittest logic for assertion checks.
@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #694 (812e46c) into main (fdf4880) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #694      +/-   ##
==========================================
- Coverage   91.86%   91.86%   -0.01%     
==========================================
  Files          17       17              
  Lines        1623     1622       -1     
==========================================
- Hits         1491     1490       -1     
  Misses        132      132              
Impacted Files Coverage Δ
papermill/engines.py 98.36% <100.00%> (-0.01%) ⬇️

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 fdf4880...812e46c. Read the comment docs.

@surdouski
Copy link
Contributor Author

@MSeal Can this be merged? Thanks.

@MSeal MSeal merged commit 54f6c03 into nteract:main Oct 18, 2022
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