[WIP] Turn into package #6
Conversation
setup.py
Outdated
| long_description = long_description, | ||
| long_description_content_type="text/x-rst", | ||
| author = "", | ||
| author_email = "FIXME", |
There was a problem hiding this comment.
Also, if I've jumped the gun here, just tell me
There was a problem hiding this comment.
I'm just catching up now, but I like the concepts...
datasets/__init__.py
Outdated
| @@ -0,0 +1,3 @@ | |||
|
|
|||
| from datasets._version import __version__ | |||
| from datasets.utils import Interface | |||
There was a problem hiding this comment.
Not sure about the import name datasets, but it can be changed easily
There was a problem hiding this comment.
I would stick with longer but more specific, so the user knows what kind of datasets, and so they are able to install alongside other packages. Maybe pip install pydicom-datasets, to then import to pydicom_datasets?
There was a problem hiding this comment.
Or seeing pydicom-data below... just call it that! :)
There was a problem hiding this comment.
Yeah, would really like to avoid the name 'dataset(s)' given the obvious confusion with DICOM datasets. Maybe data_store, file_data, DICOM_file_store, ... some combination of those kinds of words?
There was a problem hiding this comment.
If the module is called dicom store, I would expect it to also be able
to create or otherwise interact with said stores (e.g. https://cloud.google.com/healthcare/docs/how-tos/dicom). But if it’s just a data library, the name should probably reflect that and be something with data or datasets (branded for pydicom)... unless you again want to advertise that it can be used in other ways external to it.
There was a problem hiding this comment.
Changed to data_store
|
|
||
| # Backup files | ||
| *~ | ||
|
|
There was a problem hiding this comment.
I just copied it from pydicom and added a little bit extra
setup.py
Outdated
| "Programming Language :: Python :: 3.6", | ||
| "Programming Language :: Python :: 3.7", | ||
| "Programming Language :: Python :: 3.8", | ||
| "Programming Language :: Python :: 3.9", |
There was a problem hiding this comment.
should 3.9 be part of the matrix, or is this a future TODO?
There was a problem hiding this comment.
Removed, mainly just there for the future
darcymason
left a comment
darcymason
left a comment
There was a problem hiding this comment.
Looks good ... I just don't like the datasets name for sure. If we can come up with something good for that, I'm in. I'll probably add the general comments on the pydicom PR, where things are a bit more complex. But I've reviewed that too and it looks pretty good.
| self.data_path = os.path.abspath( | ||
| os.path.join(cur_dir, "../", "../", "data") | ||
| ) | ||
|
|
There was a problem hiding this comment.
Might prefer pathlib for managing the paths here, as I think it is likely easier to follow and maintain in future, but no big deal one way or the other.
datasets/__init__.py
Outdated
| @@ -0,0 +1,3 @@ | |||
|
|
|||
| from datasets._version import __version__ | |||
| from datasets.utils import Interface | |||
There was a problem hiding this comment.
Yeah, would really like to avoid the name 'dataset(s)' given the obvious confusion with DICOM datasets. Maybe data_store, file_data, DICOM_file_store, ... some combination of those kinds of words?
datasets/utils.py
Outdated
|
|
||
|
|
||
| class Interface: | ||
| """Interface to pydicom-data. |
There was a problem hiding this comment.
Depending on the other name discussion above, maybe something a little more specific for the class name: DataInterface, DataStoreInterface, even just DataStore, given any class defines an interface.
There was a problem hiding this comment.
Done, went with DataStore
darcymason
left a comment
darcymason
left a comment
There was a problem hiding this comment.
Looks good - just suggestions for converting all os.path etc. to pathlib - I think it is easier to read and would be nice not to mix os.path.X and pathlib.
| def setup(self): | ||
| cur_dir = os.path.dirname(os.path.realpath(__file__)) | ||
| self.data_path = Path(cur_dir).joinpath("..", "..", "data").resolve() | ||
|
|
There was a problem hiding this comment.
can simplify similar to other pathlib ...
3 participants
Describe the changes