Pull "ephemeral" collections into their own module #109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The meaning and usage of the various `SimpleWhatever` collections was unclear with them colocated with the abstract implementation. Moving them to their own `ephemeral` module and not having them in the main `somacore` namespace will make them less attractive. They still have value in being the most basic possible implementation of a collection, and are useful in tests for this package. In the process of removing the `SimpleCollection` export from the package root, also changes the `__init__.py` to just import module members rather than the module itself. Since this is at the top level and is exclusively for re-export, this does not have the same potential circular dependency problems as doing so within internal modules, while reducing repetition and the possibility of errors.
|
Another little note as I do some follow-up work: Another thing I have found the ephemeral types useful for is to validate the type hierarchy and make sure generics work as I expect. I also think in the future they could be more useful in testing if we had in-memory dataframes/ndarrays that wrapped pandas dataframes / numpy ndarrays and provided appropriate methods; experimentquery could be tested entirely in isolation (but that is a future thing, if ever). |
atolopko-czi
reviewed
Feb 2, 2023
|
|
||
| @property | ||
| def uri(self) -> str: | ||
| return f"somacore:simple-collection:{id(self):x}" |
There was a problem hiding this comment.
ephemeral-collection for consistency?
atolopko-czi
reviewed
Feb 2, 2023
|
|
||
|
|
||
| class SimpleCollectionTest(unittest.TestCase): | ||
| class EphemeralCollectionTest(unittest.TestCase): | ||
| def test_basic(self): | ||
| # Since the SimpleCollection implementation is straightforward this is |
There was a problem hiding this comment.
might want to s/\bSimple(.+)\b/Ephemeral$1/
There was a problem hiding this comment.
Whoops! Thank you.
atolopko-czi
approved these changes
Feb 2, 2023
johnkerl
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The meaning and usage of the various
SimpleWhatevercollections was unclear with them colocated with the abstract implementation. Moving them to their ownephemeralmodule and not having them in the mainsomacorenamespace will make them less attractive. They still have value in being the most basic possible implementation of a collection, and are useful in tests for this package.In the process of removing the
SimpleCollectionexport from the package root, also changes the__init__.pyto just import module members rather than the module itself. Since this is at the top level and is exclusively for re-export, this does not have the same potential circular dependency problems as doing so within internal modules, while reducing repetition and the possibility of errors.