add a base Track class, remove all reprs #205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 95.87% 95.97% +0.10%
==========================================
Files 18 19 +1
Lines 1938 1938
==========================================
+ Hits 1858 1860 +2
+ Misses 80 78 -2 |
😍 |
mirdata/utils.py
Outdated
|
|
||
| for attr in attributes: | ||
| val = getattr(self, attr) | ||
| if isinstance(val, str): |
There was a problem hiding this comment.
should this be if not isinstance?
There was a problem hiding this comment.
if you support py2 still (do you?) you need to check baseinstance for basestring
There was a problem hiding this comment.
I think it's right - I'm formatting strings with visible " around the values when printing
mirdata/utils.py
Outdated
|
|
||
|
|
||
| MIR_DATASETS_DIR = os.path.join(os.getenv('HOME', '/tmp'), 'mir_datasets') | ||
|
|
||
|
|
||
| class Track(object): | ||
| def __repr__(self): |
There was a problem hiding this comment.
right now you're not truncating strings or anything right? if an object has a really long value it might hang ipython.
There was a problem hiding this comment.
good point - will add this
mirdata/utils.py
Outdated
|
|
||
|
|
||
| MIR_DATASETS_DIR = os.path.join(os.getenv('HOME', '/tmp'), 'mir_datasets') | ||
|
|
||
|
|
||
| class Track(object): |
There was a problem hiding this comment.
i might have put this in track.py. utils.py is a bit of an antipattern, or at least good to avoid.
mirdata/utils.py
Outdated
| if val.__doc__ is None: | ||
| raise ValueError("{} has no documentation".format(prop)) | ||
|
|
||
| val_type_str = val.__doc__.split(':')[0] |
There was a problem hiding this comment.
do you validate that docstrings are correctly formatted?
There was a problem hiding this comment.
nope - should I? if it doesn't have a : character somewhere it'll just print the whole docstring, which is ugly but not broken
There was a problem hiding this comment.
LGTM. I concur with Rachel that it's better to test the base class rather than specific implementations. Also, i think we can live with a change in __repr__ between alpha and 0.2.0
I don't expect that users will call the __repr__ function programatically anyways.
Merge at your convenience
Implements #178 [EDIT by @lostanlen: closes #178]
All dataset
Trackclasses now inherit fromutils.Track, which has an automated implementation of__repr__, removing the need for new loaders to write this themselves 🎉It has one peculiarity, which in my opinion ends up being kind of nice -
__repr__will break if implementedTrackproperties do not have documentation, and our tests catch that.