-
Notifications
You must be signed in to change notification settings - Fork 15
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
DEP: Deprecate content as dict input, split in two arguments #1016
Conversation
ff392e6
to
046460e
Compare
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.
Certainly a good step. LGTM
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.
Nice job
def _get_content_enum(self) -> Optional[enums.Content]: | ||
"""Get the content enum.""" |
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.
Not relevant to your PR, but this is the sort of thing that, IMO, should be done as part the initializer after #914, and internal state just has the enum value
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.
agree! there is a lot of here that can be taken away once we are done with the deprecation period 🎉 and also the issue you mention will be a step in the right direction! The code will be so much leaner, can't wait!
046460e
to
78af9df
Compare
PR to start deprecating
content
as a dictionary, and introduce a newcontent_metadata
argument for the additional information.Closes #1012
Closes #1005