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
DM-20915: Add str(Formatter) and force Formatter to require an arg for constructor #178
Conversation
Update tests to match, switching some of them to using getFormatterClass so that the parameter is irrelevant.
raise TypeError("File descriptor must be a FileDescriptor") | ||
self._fileDescriptor = fileDescriptor | ||
|
||
def __str__(self): | ||
return f"{self.name()}@{self.fileDescriptor.location.path}" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it'd be straightforward to add the ideal kind of __repr__
, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was straightforward here but required I added repr() to FileDescriptor and Location and also that I cleaned up repr() for StorageClass to make it less chatty. You now get a repr that looks something like:
lsst.daf.butler.formatters.jsonFormatter.JsonFormatter(FileDescriptor(Location('/Users/timj/work/lsstsw3/build/daf_butler/a', 'B'), StorageClass('xxx'), readStorageClass=StorageClass('yyy')))
@TallJimbo you might want to take a look at all the new code now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I didn't know about {!r}
, and I'm glad that I now do.
Having it return "builtins.str" serves no useful purpose and it's much easier to return "str" and "dict".
Previously the python type to name conversion was deferred until the python type was needed. This is unnecessary and causes problems with repr() implementations. Now always extract the full name using getFullTypeName.
Make it more compact by removing entities that are defaulted.
Now consistent in mentioning it returns a Formatter class not Formatter instance.
There is no longer any gain in allowing an optional FileDescriptor argument so remove that option.