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 deleter for the kiva_backend property (which is readonly) #1686

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

mdickinson
Copy link
Member

PR #1670 added deleters for various ETSConfig attributes. However, the kiva_backend property is readonly, with value derived from the toolkit attribute, so we shouldn't be providing a deleter for it. The right way to mock kiva_backend is to set the toolkit attribute (and in fact there's already a test for that).

This PR removes the deleter for the kiva_backend attribute.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

I think that the del has use here: like the other del methods, it effectively resets the value of kiva_backend. Without this, a reset requires directly settting _kiva_backend to `None.

Unless there is a problem I am not seeing.

@mdickinson
Copy link
Member Author

kiva_backend is just a derived property of toolkit, so you can do a reset by deleting toolkit (or setting it to another value).

@mdickinson
Copy link
Member Author

Unless there is a problem I am not seeing.

Ah no, there's a problem I'm not seeing, which is that resetting toolkit should set _kiva_backend to None. Cache invalidation and all that ...

@mdickinson
Copy link
Member Author

Okay, I think this needs a closer look; I'm not at all convinced that the current code (even before the deleters were added) is behaving as it should be. I'll add some new tests and rework.

What I think we want is that the value of kiva_backend is always consistent with (and can be deduced from) the value of toolkit. The easiest way to achieve that would be simply not to cache the value: the computation of the value isn't at all time-consuming (no imports involved, for example) and this isn't something we're going to be accessing often enough for the caching to be worthwhile.

@mdickinson mdickinson marked this pull request as draft August 8, 2022 16:06
@mdickinson
Copy link
Member Author

Updated to make kiva_backend uncached.

@mdickinson mdickinson marked this pull request as ready for review August 9, 2022 08:12
Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

I think that this is reasonable.

@mdickinson mdickinson merged commit aa05dc6 into main Aug 9, 2022
@mdickinson mdickinson deleted the fix/remove-kiva-backend-property-deleter branch August 9, 2022 13:03
mdickinson added a commit that referenced this pull request Aug 9, 2022
PR #1670 added deleters for various ETSConfig attributes. However, the kiva_backend property is readonly, with value derived from the toolkit attribute, so we shouldn't be providing a deleter for it. The right way to mock kiva_backend is to set the toolkit attribute (and in fact there's already a test for that).

This PR removes the deleter for the kiva_backend attribute, and makes the `kiva_backend` property getter a pure function with no caching.

(cherry picked from commit aa05dc6)
mdickinson added a commit that referenced this pull request Aug 9, 2022
PR #1670 added deleters for various ETSConfig attributes. However, the kiva_backend property is readonly, with value derived from the toolkit attribute, so we shouldn't be providing a deleter for it. The right way to mock kiva_backend is to set the toolkit attribute (and in fact there's already a test for that).

This PR removes the deleter for the kiva_backend attribute, and makes the `kiva_backend` property getter a pure function with no caching.

(cherry picked from commit aa05dc6)
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