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

Implement singleton state and use it in the 2d annotator #240

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

constantinpape
Copy link
Contributor

@constantinpape constantinpape commented Oct 23, 2023

Fixes #201

@constantinpape constantinpape changed the base branch from master to dev October 23, 2023 21:51
@constantinpape constantinpape marked this pull request as ready for review October 24, 2023 19:57
@constantinpape
Copy link
Contributor Author

constantinpape commented Oct 24, 2023

Hi @GenevieveBuckley ,
the state refactoring is good to go now. If you want you can quickly review if this also makes sense for you. If yes, feel free to merge this.
I think with these changes you can then go ahead with #235.

I have tested the annotator tools and they all still work as expected.

Copy link
Collaborator

@GenevieveBuckley GenevieveBuckley left a comment

Choose a reason for hiding this comment

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

I keep coming back to this because I'm not sure I can make a meaningful review. Sorry!

This implementation looks like the metaclass singleton (as described here), so it seems like a reasonable approach. But I don't have enough expertise to really know if this is definitely the best implementation approach, or assess any of the discussion around singletons in python.

I'd like to say, let's just try it and see what breaks. The test coverage isn't great, so I'm not hugely confident that everything will go smoothly. But on the other hand, we need to move forward. So I think we should go ahead and accept that maybe some unexpected problems might pop up.

(The alternative is for me to pull this branch into my working branch, and check if singleton + embedding widget work well together. The disadvantage there is that multiple small PRs are usually quicker to get merged than one or two giant PRs).

@constantinpape
Copy link
Contributor Author

Thanks for the review @GenevieveBuckley!

This implementation looks like the metaclass singleton (as described here), so it seems like a reasonable approach. But I don't have enough expertise to really know if this is definitely the best implementation approach, or assess any of the discussion around singletons in python.

Yes, it follows the metaclass pattern. I don't use inheritance to have a separate implementation class that is not yet a singleton as in the example. I don't think it's needed for us, but it could be added later if required.

I'd like to say, let's just try it and see what breaks. The test coverage isn't great, so I'm not hugely confident that everything will go smoothly. But on the other hand, we need to move forward. So I think we should go ahead and accept that maybe some unexpected problems might pop up.

Ok, let's go ahead with it! Re test coverage: I have added two unit-tests for the state now. And for high-level / integration tests we will need to wait till the plugin structure takes more shape. I don't think it makes too much sense to add many tests for the current tool design, as this will change a bit to accommodate the plugins.

(The alternative is for me to pull this branch into my working branch, and check if singleton + embedding widget work well together. The disadvantage there is that multiple small PRs are usually quicker to get merged than one or two giant PRs).

Yes, I also prepare having smaller PRs.

@constantinpape constantinpape merged commit 411d48f into dev Oct 30, 2023
1 check passed
@constantinpape constantinpape deleted the singleton branch October 30, 2023 09:17
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.

2 participants