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

feature: lockable mixin #215

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

feature: lockable mixin #215

wants to merge 6 commits into from

Conversation

psomhorst
Copy link
Contributor

While working on tests for ContinuousData, I was fixing some locking mechanisms. Since these will probably be reused in the future, I decided to write a mixin instead.

This mixin has been implemented and tested with ContinuousData only.

@psomhorst
Copy link
Contributor Author

On naming: the Lockable mixin class and the lock(), unlock(), islocked() and islockable() methods might suggest that the entire object is made lockable. This is not the case. Attributes in the object are lockable. We could rename this to:

  • lock_attr()
  • unlock_attr()
  • attr_is_locked()
  • attr_is_lockable()

but that becomes quite verbose.

@psomhorst psomhorst changed the title 213 lockable mixin feature: lockable mixin Jun 7, 2024
@psomhorst psomhorst changed the base branch from main to 167_sparse_data_psomhorst June 7, 2024 08:41
@psomhorst psomhorst marked this pull request as draft June 7, 2024 08:42
Base automatically changed from 167_sparse_data_psomhorst to main June 7, 2024 09:25
@DaniBodor DaniBodor mentioned this pull request Jun 10, 2024
@DaniBodor DaniBodor linked an issue Jun 10, 2024 that may be closed by this pull request
@DaniBodor
Copy link
Member

On naming: the Lockable mixin class and the lock(), unlock(), islocked() and islockable() methods might suggest that the entire object is made lockable. This is not the case.

I think it's fine to use this nomenclature. I don't think the alternatives you suggest make it that much clearer. While it is formally speaking more accurate, I don't think it would be interpreted differently by a reader. We need to document well what locked (etc) means, or find a way to make the entire object lockable.

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.

Create Lockable mixin
2 participants