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-15850: Always import default storage classes #89
Conversation
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.
Some comments to consider, but I am fine with the solution to this problem
-------------- | ||
{sep.join(f"{s}: {self._storageClasses[s]}" for s in self._storageClasses)} | ||
""" | ||
|
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.
This would be nicer if the text was in a similar formatting, so maybe:
returnString = f"""
Number of registered StorageClasses: {len(self._storageClasses)}
StorageClasses
--------------
{sep.join(f"{s}: {self._storageClasses[s]}" for s in self._storageClasses)}
"""
return textwrap.dedent(returnString.lstrip("\n"))
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.
I don't really like all the dedent shenanigans and I don't have a problem with the left justification so I'd prefer to leave it as is.
@@ -335,9 +335,29 @@ def __init__(self, config=None): | |||
self._storageClasses = {} | |||
self._configs = [] | |||
|
|||
# Always seed with the default config | |||
c = StorageClassConfig() |
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.
In general I think single letter variable names are not good, even in simple cases. If this was named something like defaultConfiguration, there would be no need for the comment. Alternatively, there is little point in naming the output with a single character to just consume it. This could be self.addFromConfig(StorageClassConfig())
with the comment left above.
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.
I'll remove the variable. defaultConfiguration
seems like a very long variable name for such a short-lived variable.
Mostly expected to be useful for people trying to determine what storage classes have been registered for debugging purposes.
d5959e5
to
3afeb32
Compare
No description provided.