bpo-34926: Make mimetypes.guess_type accept os.PathLike objects #9777

mayankasthana · 2018-10-09T18:22:28Z

https://bugs.python.org/issue34926

the-knights-who-say-ni · 2018-10-09T18:22:31Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

tirkarthi · 2018-10-09T18:25:35Z

Lib/test/test_mimetypes.py

+
+        expected = self.db.guess_type(filename)
+
+        eq(self.db.guess_type(filepath), expected)


Please use self.assertEqual instead of using alias to be consistent with rest of the tests.

tirkarthi · 2018-10-09T18:29:36Z

Thanks for the PR. Adding a NEWS entry helps since this is an enhancement. Please refer to https://devguide.python.org/committing/#what-s-new-and-news-entries

pitrou

Thanks @mayankasthana. As @tirkarthi said, you need to add a NEWS entry for this change. Also, you need to sign the Python CLA if you haven't already done so (see automated message above).

As for the PR itself, here a couple review comments.

pitrou · 2018-10-09T18:33:40Z

Lib/mimetypes.py

@@ -113,6 +113,10 @@ def guess_type(self, url, strict=True):
        Optional `strict' argument when False adds a bunch of commonly found,
        but non-standard types.
        """
+        url = url_or_path
+        if isinstance(url_or_path, os.PathLike):


You probably don't need the conditional here, url = os.fspath(url) also works on strings.

pitrou · 2018-10-09T18:34:14Z

Lib/mimetypes.py

-    def guess_type(self, url, strict=True):
-        """Guess the type of a file based on its URL.
+    def guess_type(self, url_or_path, strict=True):
+        """Guess the type of a file which can either be a url or an os.PathLike object.


Can you keep "URL" uppercase? Also, this line seems a bit long (though Github doesn't display the line length).

pitrou · 2018-10-09T18:34:41Z

Lib/test/test_mimetypes.py

@@ -3,6 +3,7 @@
 import mimetypes
 import sys
 import unittest
+import pathlib


Can you please keep imports in alphebatical order?

bedevere-bot · 2018-10-09T18:36:38Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tirkarthi · 2018-10-09T18:37:58Z

Doc can be updated at https://docs.python.org/3/library/mimetypes.html#mimetypes.guess_type . I think Changed in version 3.8: Accepts a path-like object. can be used similar to doc at https://docs.python.org/3/library/gzip.html#gzip.GzipFile . I will let the reviewers to decide on the exact wording to be used.

pitrou · 2018-10-09T18:39:41Z

@tirkarthi You're right, I forgot about the necessary doc changes.

mayankasthana · 2018-10-09T19:23:16Z

I have made the requested changes; please review again.

Also, I have signed the CLA, might take a little for the status to update.

bedevere-bot · 2018-10-09T19:23:19Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou

Thanks for doing the changes. I have two more nits below.

pitrou · 2018-10-09T19:51:50Z

Doc/library/mimetypes.rst


   .. index:: pair: MIME; headers

-   Guess the type of a file based on its filename or URL, given by *url*.  The
-   return value is a tuple ``(type, encoding)`` where *type* is ``None`` if the
+   Guess the type of a file based on its filename/URL or by its path (any :class:`os.PathLike`).


I would phrase it this way:

Guess the type of a file based on its filename, path or URL, given by *url*. *url* can be a string or a :term:`path-like object`.

pitrou · 2018-10-09T19:53:26Z

Lib/mimetypes.py

-    def guess_type(self, url, strict=True):
-        """Guess the type of a file based on its URL.
+    def guess_type(self, url_or_path, strict=True):
+        """Guess the type of a file which is either a URL or an os.PathLike object.


I would say "path-like object" rather than "os.PathLike object", since os.PathLike isn't something that users instantiate explicitly.
Also, I think the argument name can be kept as url rather than url_or_path. People may be passing the argument by name.

bedevere-bot · 2018-10-09T19:54:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mayankasthana · 2018-10-10T08:24:58Z

I hadn't realized that I had changed the API. Changed it back. Also added a test when named arguments are used.
Also made the doc changes.

I have made the requested changes; please review again.

bedevere-bot · 2018-10-10T08:25:01Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou · 2018-10-10T14:46:09Z

Thanks for polishing this! The PR is good to go now.

mayankasthana · 2018-10-11T13:31:14Z

Thanks @pitrou and @tirkarthi.

tirkarthi · 2018-10-11T13:36:04Z

@mayankasthana You're welcome. Looking forward to more contributions :)

Make mimetypes.guess_type accept os.PathLike objects

4ee6d03

mayankasthana requested a review from a team as a code owner October 9, 2018 18:22

the-knights-who-say-ni added the CLA not signed label Oct 9, 2018

bedevere-bot added the awaiting review label Oct 9, 2018

tirkarthi reviewed Oct 9, 2018

View reviewed changes

Update assert in test

1271541

pitrou added the type-feature A feature request or enhancement label Oct 9, 2018

pitrou requested changes Oct 9, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 9, 2018

mayankasthana added 5 commits October 10, 2018 00:23

Add news blurb

b76531e

Shorten docstring, capitalize URL

b8e7462

Remove unneeded conditional

bf78ecc

Rearrange imports in alphabetical order

ccac055

Update library documentation

0bf4f30

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 9, 2018

pitrou requested changes Oct 9, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Oct 9, 2018

mayankasthana added 2 commits October 10, 2018 11:34

Bring back original API, add keyword args tests

8efefb8

Update documentation

ec8c08a

Fix undefined term path-like

45e5600

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 10, 2018

Fix docs build

ea3d6ca

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 10, 2018

pitrou approved these changes Oct 10, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Oct 10, 2018

pitrou merged commit 7e18dee into python:master Oct 10, 2018

bedevere-bot removed the awaiting merge label Oct 10, 2018


		expected = self.db.guess_type(filename)

		eq(self.db.guess_type(filepath), expected)

Uh oh!

bpo-34926: Make mimetypes.guess_type accept os.PathLike objects #9777

bpo-34926: Make mimetypes.guess_type accept os.PathLike objects #9777

Uh oh!

Conversation

mayankasthana commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Oct 9, 2018

Uh oh!

tirkarthi Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tirkarthi commented Oct 9, 2018

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 9, 2018

Uh oh!

tirkarthi commented Oct 9, 2018

Uh oh!

pitrou commented Oct 9, 2018

Uh oh!

mayankasthana commented Oct 9, 2018

Uh oh!

bedevere-bot commented Oct 9, 2018

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 9, 2018

Uh oh!

mayankasthana commented Oct 10, 2018

Uh oh!

bedevere-bot commented Oct 10, 2018

Uh oh!

pitrou commented Oct 10, 2018

Uh oh!

mayankasthana commented Oct 11, 2018

Uh oh!

tirkarthi commented Oct 11, 2018

Uh oh!

Uh oh!

mayankasthana commented Oct 9, 2018 •

edited

Loading

tirkarthi Oct 9, 2018 •

edited

Loading