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

Use not mutable fields on Viewer where appropriate #2346

Merged
merged 3 commits into from Mar 6, 2021

Conversation

sofroniewn
Copy link
Contributor

Description

Following the release of pydantic 1.8.x and pydantic/pydantic#2196 we can now use field not mutable fields where appropriate on our Viewer. This will prevent people from overwriting the dims, or trying to set things that cannot be set. We also won't add event emitters for these fields as they cannot be changed.

Technically this is a breaking change, but I'm not quite sure how anyone would have successfully set these things before!

It should be reviewed after we fix our CI with 1.8.1 (see #2322). Note this will bump our min pydantic requirement to 1.8.1 as allow_mutation=False was previously not supported.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@sofroniewn sofroniewn added the breaking change Breaking change requiring a deprecation notice label Mar 4, 2021
@sofroniewn sofroniewn added this to the 0.4.7 milestone Mar 4, 2021
Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

I like it! but is pydantic >1.8.1 required for this?

@sofroniewn
Copy link
Contributor Author

I like it! but is pydantic >1.8.1 required for this?

Yes I'm pretty sure, the allow_mutation kwarg wasn't added until 1.8.0 see pydantic/pydantic#2196 so unless they backported it, it shouldn't work - but I havn't tried it!

@tlambert03
Copy link
Member

eh, fine with me either way. since we're already >=1.7.3, I'm not sure that single version is all that important

@codecov
Copy link

codecov bot commented Mar 6, 2021

Codecov Report

Merging #2346 (5faafb1) into master (5a2b3c3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2346   +/-   ##
=======================================
  Coverage   80.85%   80.86%           
=======================================
  Files         433      433           
  Lines       35724    35736   +12     
=======================================
+ Hits        28886    28898   +12     
  Misses       6838     6838           
Impacted Files Coverage Δ
napari/components/_tests/test_viewer_model.py 100.00% <100.00%> (ø)
napari/components/viewer_model.py 94.24% <100.00%> (ø)
napari/utils/events/evented_model.py 96.47% <100.00%> (+0.17%) ⬆️

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 5a2b3c3...5faafb1. Read the comment docs.

@sofroniewn
Copy link
Contributor Author

eh, fine with me either way. since we're already >=1.7.3, I'm not sure that single version is all that important

Ok nice, I will merge this now. I think we then have a fresh start with >= 1.8.1. Thanks!!

@sofroniewn sofroniewn merged commit ab791bc into napari:master Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Breaking change requiring a deprecation notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants