-
Notifications
You must be signed in to change notification settings - Fork 62
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
SG-36835 Python 2 removal #197
base: master
Are you sure you want to change the base?
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.
Looks good to me. I've pointed out some usages of ensure_str
that can be replaced by str
. If it's not possible then there's no problem :)
hooks/collector.py
Outdated
@@ -429,7 +433,7 @@ def _get_item_info(self, path): | |||
# the system's default encoding. If a unicode string is | |||
# returned, we simply ensure it's utf-8 encoded to avoid issues | |||
# with toolkit, which expects utf-8 | |||
category_type = six.ensure_str(category_type) | |||
category_type = sgutils.ensure_str(category_type) |
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.
It seems category_type
can be a Optional[str]
type according to the documentation of guess_type
function: https://docs.python.org/3.9/library/mimetypes.html#mimetypes.guess_type. So I think it can be safe to use just str
.
The main difference here with ensure_str
is that this function will raise an error when the argument is None
, instead of converting None
to 'None'
.
@@ -399,7 +403,7 @@ def _get_item_info(self, path): | |||
# the system's default encoding. If a unicode string is | |||
# returned, we simply ensure it's utf-8 encoded to avoid issues | |||
# with toolkit, which expects utf-8 | |||
category_type = six.ensure_str(category_type) | |||
category_type = sgutils.ensure_str(category_type) |
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.
Same as my previous comment. This is a unit test so here is the perfect chance to check some use cases.
Description
Python 2 removal
Test
Analyzes the given file and creates one or more items to represent it.
Makes sure that the right hand side details section reflects the selected item in the left hand side tree.
When someone drops stuff into the publish.