GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-31285: fix an assertion failure and a SystemError in warnings.warn_explicit by orenmn · Pull Request #3219 · python/cpython

Conversation

orenmn commented Aug 27, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

in _warnings.c: add checks whether the return value of the received loader's get_source() is invalid.
in test_warnings/__init__.py: add tests to verify the assertion failure and the SystemError are no more.

https://bugs.python.org/issue31285

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orenmn added 2 commits

August 27, 2017 20:18


          init commit

ff52f6b


          add a NEWS.d item

252647d

the-knights-who-say-ni added the CLA signed label

Aug 27, 2017

bedevere-bot added the awaiting review label

Aug 27, 2017

orenmn added 2 commits

August 28, 2017 13:06


          replace the C equivalent of source.splitlines() with str.splitlines(s…

d4dbe6d

…ource)


          fix NEWS.d item

9eb55b5

serhiy-storchaka reviewed

Aug 30, 2017

View reviewed changes

Comment thread

Python/_warnings.c Outdated


		/* Split the source into lines. */
		source_list = PyObject_CallMethodObjArgs(source,
		source_list = PyObject_CallMethodObjArgs((PyObject *)&PyUnicode_Type,

serhiy-storchaka Aug 30, 2017

Copy link

Copy Markdown

Member

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.

Use just PyUnicode_Splitlines().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_warnings/__init__.py Outdated

		return splitlines_ret_val
		return BadSource()
		return BadLoader()
		self.assertRaises(IndexError, self.module.warn_explicit,

serhiy-storchaka Aug 30, 2017

Copy link

Copy Markdown

Member

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.

Since this test already takes multiple lines, I think it would look clearer if use a context manager form:

with self.assertRaises(IndexError):
     self.module.warn_explicit(...)

But why IndexError is expected? Wouldn't be better to test the normal case that doesn't raise an error? You can use test.support.captured_stderr() for capturing the warning output.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_warnings/__init__.py Outdated

		self.assertNotIn(b'Warning!', stderr)
		self.assertNotIn(b'Error', stderr)

		def test_warn_explicit(self):

serhiy-storchaka Aug 30, 2017

Copy link

Copy Markdown

Member

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 name is too general, but the test isn't a general test of warn_explicit(). Use something more specific like test_issue31285.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          improve test method and its name, and change to PyUnicode_Splitlines()

orenmn commented

Aug 30, 2017

View reviewed changes

Comment thread

Lib/test/test_warnings/__init__.py Outdated

		'foo', UserWarning, 'bar', 1,
		module_globals={'__loader__': _get_bad_loader(42),
		'__name__': 'foobar'})
		self.assertIn('bar:1: UserWarning: foo', stderr.getvalue())

orenmn Aug 30, 2017

Copy link

Copy Markdown

Contributor Author

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 am not sure whether this and the other call to assertIn() are necessary, as we want to make sure a SystemError and an assertion failure don't happen...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting core review and removed awaiting review labels

Aug 30, 2017


          resolve conflict in test_warnings/__init__.py

b8fe794

serhiy-storchaka reviewed

Sep 24, 2017

View reviewed changes

Comment thread

Lib/test/test_warnings/__init__.py Outdated

		# warn_explicit() should neither raise a SystemError nor cause an
		# assertion failure, in case the return value of get_source() has a
		# bad splitlines() method.
		def _get_bad_loader(splitlines_ret_val):

serhiy-storchaka Sep 24, 2017

Copy link

Copy Markdown

Member

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.

Starting underscore is not needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

orenmn reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Lib/test/test_warnings/__init__.py Outdated

		del self.module._showwarnmsg
		with support.captured_stderr() as stderr:
		self.module.warn_explicit(
		'foo', ArithmeticError, 'bar', 1,

serhiy-storchaka Sep 24, 2017

Copy link

Copy Markdown

Member

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.

Is it valid to use ArithmeticError which is not a warning category?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orenmn Sep 24, 2017

Copy link

Copy Markdown

Contributor Author

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 is valid (IIUC, only warning.warns() requires the category to be a subclass of Warning). I used ArithmeticError because it caused get_filter() to return 'default'. But now it seems quite obscure to me, so I would change it.

Also, my test fails with -Werror, so I would fix that too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orenmn added 3 commits

September 24, 2017 14:52


          merge master

64f31fc


          remove redundant underscore

6fe73a4


          fix the test, and make it clearer

71da7df

serhiy-storchaka added needs backport to 2.7 type-bug An unexpected behavior, bug, or error labels

Sep 24, 2017

serhiy-storchaka approved these changes

Sep 24, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels

Sep 24, 2017

serhiy-storchaka merged commit 91fb0af into python:master

Sep 24, 2017

miss-islington commented Sep 24, 2017

Copy link

Copy Markdown

Contributor

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented Sep 24, 2017

Copy link

Copy Markdown

Contributor

Sorry, @orenmn and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 91fb0afe181986b48abfc6092dcca912b39de51d 2.7

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added needs backport to 3.6 and removed needs backport to 3.6 labels

Sep 26, 2017

miss-islington commented Sep 26, 2017

Copy link

Copy Markdown

Contributor

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented Sep 26, 2017

Copy link

Copy Markdown

Contributor

Sorry @orenmn and @serhiy-storchaka, I had trouble checking out the 3.6 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 91fb0afe181986b48abfc6092dcca912b39de51d 3.6

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented Sep 26, 2017

Copy link

Copy Markdown

Contributor

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request

Sep 26, 2017


          [3.6] bpo-31285: Fix an assertion failure and a SystemError in warnin…

…gs.warn_explicit. (pythonGH-3219)

(cherry picked from commit 91fb0af)

bedevere-bot commented Sep 26, 2017

Copy link

Copy Markdown

GH-3775 is a backport of this pull request to the 3.6 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.6 label

Sep 26, 2017

serhiy-storchaka added a commit that referenced this pull request

Sep 27, 2017


          [3.6] bpo-31285: Fix an assertion failure and a SystemError in warnin…

90fe25a

…gs.warn_explicit. (GH-3219) (#3775)

(cherry picked from commit 91fb0af)

orenmn mentioned this pull request

Sep 28, 2017

bpo-31285: Remove splitlines identifier from Python/_warnings.c (forgot to remove it in #3219) #3803

Merged

bedevere-bot commented Sep 28, 2017

Copy link

Copy Markdown

GH-3805 is a backport of this pull request to the 2.7 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 2.7 label

Sep 28, 2017

orenmn mentioned this pull request

Sep 29, 2017

[2.7] bpo-31285: fix an assertion failure and a SystemError in warnings.warn_explicit (GH-3219) #3805

Closed

miss-islington mentioned this pull request

Sep 29, 2017

[3.6] bpo-31285: Remove splitlines identifier from Python/_warnings.c (GH-3803) #3829

Merged

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 29, 2017


          [3.6] bpo-31285: Remove splitlines identifier from Python/_warnings.c (…

3c3e62c

…pythonGH-3803)

(forgot to remove it in pythonGH-3219)
(cherry picked from commit 8b4ff53)

serhiy-storchaka pushed a commit that referenced this pull request

Sep 29, 2017


          bpo-31285: Remove splitlines identifier from Python/_warnings.c (#3803)

8b4ff53

(forgot to remove it in #3219)

serhiy-storchaka pushed a commit that referenced this pull request

Sep 29, 2017


          [3.6] bpo-31285: Remove splitlines identifier from Python/_warnings.c (…

66c2b9f

…GH-3803) (#3829)

(forgot to remove it in GH-3219)
(cherry picked from commit 8b4ff53)

orenmn added a commit to orenmn/cpython that referenced this pull request

Sep 30, 2017


          [2.7] bpo-31285: Don't raise a SystemError in warnings.warn_explicit(…

c15236b

…) in case __loader__.get_source() has a bad splitlines() method. (pythonGH-3219)

serhiy-storchaka pushed a commit that referenced this pull request

Sep 30, 2017


          [2.7] bpo-31285: Don't raise a SystemError in warnings.warn_explicit(…

40d736b

…) in case __loader__.get_source() has a bad splitlines() method. (GH-3219) (#3823)

(cherry picked from commit 91fb0af)

Mariatta removed the awaiting merge label

Oct 8, 2017

Reviewers

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

type-bug

An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

orenmn commented Aug 27, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 24, 2017

Uh oh!

miss-islington commented Sep 24, 2017

Uh oh!

miss-islington commented Sep 26, 2017

Uh oh!

miss-islington commented Sep 26, 2017

Uh oh!

miss-islington commented Sep 26, 2017

Uh oh!

bedevere-bot commented Sep 26, 2017

Uh oh!

bedevere-bot commented Sep 28, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

orenmn commented Aug 27, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.