bpo-31292: Fix distutils check --restructuredtext for include directives #3248

flying-sheep · 2017-08-30T17:45:42Z

https://bugs.python.org/issue31292

…tives

merwok · 2017-08-30T17:49:31Z

Lib/distutils/tests/test_check.py

@@ -99,6 +100,11 @@ def test_check_restructuredtext(self):
        cmd = self._run(metadata, strict=1, restructuredtext=1)
        self.assertEqual(cmd._warnings, 0)

+        # check that includes work to test #31292
+        metadata['long_description'] = f'title\n=====\n\n.. include:: {os.devnull}'


Could you use explicit formatting, so that backporting the fix becomes easier?

sure, will do in a second.

merwok · 2017-08-30T17:49:31Z

Lib/distutils/command/check.py

@@ -120,7 +120,7 @@ def check_restructuredtext(self):

    def _check_rst_data(self, data):
        """Returns warnings when the provided data doesn't compile."""
-        source_path = StringIO()
+        source_path = self.distribution.script_name or 'long_description'


Do you think it would be better to have a name looking like <something>, to hint that this is not a real filename?

(I forget which stdlib module uses <stdin> but I like the convetion)

I like the convetion

me too! the rST data is always retrieved via self.distribution.get_long_description(), so some variant of long_description seems to be most informative to me.

maybe something like this?

source_path = '<long_description>' if self.distribution.script_name: source_path = '{} {}'.format(self.distribution.script_name, source_path)

That depends if source_path has to be a path-like string, or arbitrary text.

I see your point that a warning or error message saying only «setup.py: bad include bla bla» may not be enough to let the user understand that the issue is reST in long_description, especially if the text is read from a README or other file.

If source_path is for human display only and doesn’t have to look like a file path, then +1 to your idea, maybe with parens rather than brackets: setup.py (long_description) / fallback distutils (long_description)

at multiple points the attribute docs say something like “Path to or description of the input source being processed”, which makes me assume that free-form text is OK.

merwok · 2017-08-30T18:14:48Z

Lib/distutils/command/check.py

@@ -135,7 +135,7 @@ def _check_rst_data(self, data):
                          error_handler=settings.error_encoding_error_handler)

        document = nodes.document(settings, reporter, source=source_path)
-        document.note_source(source_path, -1)
+        document.note_source(StringIO(), -1)


What does note_source do?

it sets current_source and current_line, the former also being “Path to or description of the input source being processed”

will change

Would it be easy to add a test for this too? @flying-sheep

so distutils went against this recommendation and instantiates the class directly.

new_document does exactly the same thing except that it also creates a noisy reporter instead of a silent one, so i think we can skip a test here.

note that decode_path is just something like ensure_unicode and while new_document talks of paths, the document class says “Path to or description of the input source being processed”

Ok but I would just like to be sure about what the change does. Is the source_path used in the error message or warning text maybe? Even without a unit test, does the command-line output make sense for a human reader?

@flying-sheep ping

sorry for the delay! i looked into it, and it needs to be a path after all if you use a relative path for a include or csv_table directive. i adjusted the test accordingly

merwok · 2017-08-30T18:55:15Z

I’m not sure that Travis or buildbots have docutils installed to test things, but I assume you tested locally.

Thanks for the patch! LGTM

flying-sheep · 2017-08-30T19:02:31Z

yup, sure did.

thanks for the quick and pleasant review process!

merwok

One question left

bedevere-bot · 2017-09-01T16:40:42Z

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 didn't expect the Spanish Inquisition!. 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.

flying-sheep · 2017-09-02T11:23:15Z

Answered above. I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-02T11:23:17Z

Nobody expects the Spanish Inquisition!

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

merwok · 2017-09-20T21:59:20Z

Lib/distutils/tests/test_check.py

@@ -13,6 +14,10 @@
    pygments = None


+HERE = Path(__file__).parent
+INCLUDE_TEST_PATH = (HERE / 'includetest.rst').relative_to(Path.cwd())


Would you mind using os.path functions to make it easier to backport the patch?

merwok · 2017-09-20T21:59:20Z

Lib/distutils/tests/test_check.py

+        # check that includes work to test #31292
+        metadata['long_description'] = 'title\n=====\n\n.. include:: {}'.format(INCLUDE_TEST_PATH)
+        cmd = self._run(metadata, strict=1, restructuredtext=1)
+        self.assertEqual(cmd._warnings, 0)


Is it possible to test the value of the long description (to see the desired text included)?

bedevere-bot · 2017-09-20T21:59:22Z

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 didn't expect the Spanish Inquisition!. 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.

merwok · 2017-09-20T22:01:27Z

I didn’t notice that the PR was for 3.6. Could you rebase the changes for master?

ned-deily · 2018-06-07T03:47:47Z

@flying-sheep Thanks for the PR. As @merwok noted earlier, the PR needs to be based against the cpython "master" branch, not the "3.6" branch. As I don't think there is any practical way to modify the existing PR, I'm closing this one; please resubmit a new PR against master. Thanks!

https://devguide.python.org/pullrequest/

bpo-31292: Fixed distutils check --restructuredtext for include direc…

41beaf4

…tives

the-knights-who-say-ni added the CLA signed label Aug 30, 2017

bedevere-bot added the awaiting review label Aug 30, 2017

merwok requested changes Aug 30, 2017

View changes

bedevere-bot added awaiting core review and removed awaiting review labels Aug 30, 2017

switched away from f-literal

cb0c3e5

merwok reviewed Aug 30, 2017

View changes

flying-sheep added 3 commits Aug 30, 2017

use source_path also for note_source()

9de80f5

Use consensus format for source_desc

51e34b5

added news blurb

c011a69

merwok approved these changes Aug 30, 2017

View changes

merwok self-assigned this Sep 1, 2017

merwok removed the awaiting core review label Sep 1, 2017

merwok requested changes Sep 1, 2017

View changes

bedevere-bot added the awaiting changes label Sep 1, 2017

bedevere-bot removed the awaiting changes label Sep 2, 2017

bedevere-bot added the awaiting change review label Sep 2, 2017

it needs to be a path after all

21a00df

merwok requested changes Sep 20, 2017

View changes

bedevere-bot added awaiting changes and removed awaiting change review labels Sep 20, 2017

merwok changed the base branch from 3.6 to master Sep 20, 2017

merwok changed the base branch from master to 3.6 Sep 20, 2017

ned-deily closed this Jun 7, 2018

flying-sheep mentioned this pull request Nov 20, 2018

bpo-31292: Fixed distutils check --restructuredtext for include directives #10605

Merged

bpo-31292: Fix distutils check --restructuredtext for include directives #3248

bpo-31292: Fix distutils check --restructuredtext for include directives #3248

flying-sheep commented Aug 30, 2017 •

edited

merwok Aug 30, 2017

flying-sheep Aug 30, 2017

merwok Aug 30, 2017

flying-sheep Aug 30, 2017 •

edited

merwok Aug 30, 2017

flying-sheep Aug 30, 2017

merwok Aug 30, 2017

flying-sheep Aug 30, 2017

merwok Aug 30, 2017 •

edited

flying-sheep Sep 2, 2017 •

edited

merwok Sep 3, 2017

merwok Sep 14, 2017

flying-sheep Sep 18, 2017 •

edited

merwok commented Aug 30, 2017 •

edited

flying-sheep commented Aug 30, 2017 •

edited

merwok left a comment

bedevere-bot commented Sep 1, 2017

flying-sheep commented Sep 2, 2017 •

edited

bedevere-bot commented Sep 2, 2017

merwok Sep 20, 2017

merwok Sep 20, 2017

bedevere-bot commented Sep 20, 2017

merwok commented Sep 20, 2017

ned-deily commented Jun 7, 2018

bpo-31292: Fix distutils check --restructuredtext for include directives #3248

bpo-31292: Fix distutils check --restructuredtext for include directives #3248

Conversation

flying-sheep commented Aug 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flying-sheep Aug 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merwok Aug 30, 2017 • edited

Choose a reason for hiding this comment

flying-sheep Sep 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flying-sheep Sep 18, 2017 • edited

Choose a reason for hiding this comment

merwok commented Aug 30, 2017 • edited

flying-sheep commented Aug 30, 2017 • edited

merwok left a comment

bedevere-bot commented Sep 1, 2017

flying-sheep commented Sep 2, 2017 • edited

bedevere-bot commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 20, 2017

merwok commented Sep 20, 2017

ned-deily commented Jun 7, 2018

flying-sheep commented Aug 30, 2017 •

edited

flying-sheep Aug 30, 2017 •

edited

merwok Aug 30, 2017 •

edited

flying-sheep Sep 2, 2017 •

edited

flying-sheep Sep 18, 2017 •

edited

merwok commented Aug 30, 2017 •

edited

flying-sheep commented Aug 30, 2017 •

edited

flying-sheep commented Sep 2, 2017 •

edited