Skip to content

Improve display of continuation lines with multiline errors #1807

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

Merged

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Aug 11, 2016

Fixes #717.
Follow-up to #1762.

@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.0009%) to 92.324% when pulling bf1313a on blueyed:improve-multiline-error-format into 34925a3 on pytest-dev:master.

@nicoddemus
Copy link
Member

Hey @blueyed. thanks!

Looks good to me, perhaps though we should add a CHANGELOG entry for that as well.

prefix = ' '
tw.line(prefix + line.strip(), red=True)
if lines:
tw.line('E ' + lines[0].strip(), red=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that we have this in py._code.code.py:

class FormattedExcinfo(object):
    """ presenting information about failing Functions and Generators. """
    # for traceback entries
    flow_marker = ">"
    fail_marker = "E"

Could please change the code use those constants instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Not sure though if that is really good like that.
After all it should be some more generic method probably for this kind of messages?

@tomviner
Copy link
Contributor

LGTM 👍

@blueyed blueyed force-pushed the improve-multiline-error-format branch 2 times, most recently from 29ec412 to 998f9a5 Compare August 14, 2016 20:04
@blueyed blueyed force-pushed the improve-multiline-error-format branch from 998f9a5 to c163cc7 Compare August 14, 2016 20:34
@nicoddemus nicoddemus merged commit d58a8e3 into pytest-dev:master Aug 15, 2016
@nicoddemus
Copy link
Member

Thanks @blueyed and @tomviner!

@blueyed blueyed deleted the improve-multiline-error-format branch August 15, 2016 22:11
@blueyed
Copy link
Contributor Author

blueyed commented Aug 16, 2016

FWIW: #1762 should have been made against the features branch probably. There are huge conflicts now, since it was split in 8c49561.

@blueyed
Copy link
Contributor Author

blueyed commented Aug 16, 2016

@nicoddemus
In case you don't know about rerere already: https://git-scm.com/blog/2010/03/08/rerere.html (outdated, better check the man page probably).

@nicoddemus
Copy link
Member

Thanks for the heads up @blueyed!

@tomviner
Copy link
Contributor

@blueyed said:

FWIW: #1762 should have been made against the features branch probably.

That's probably by bad, I was advising new contributors at the Europython sprint 😬

Looking again at #717 (which #1762 attempts to fix) it's more of a "bring into line" (with other traceback output), than strictly a "bugfix". So I guess default to features branch for these kind of tickets?

@nicoddemus
Copy link
Member

@tomviner not sure, that's such a small change in output, I think it is OK for it to go to master. Others may have other opinions though. 😁

@The-Compiler
Copy link
Member

I'm still wondering what crazy output parsing people do (like for IDEs) though, and how much we break even with small output changes.

@tomviner
Copy link
Contributor

@The-Compiler I'm sure they do all sorts. But if we have faith in the longevity of pytest, perhaps being more consistent now will help future crazy output parsers ;-)

@The-Compiler
Copy link
Member

@tomviner Sure, but I think that's a good reason to have any changes to the output in features 😉

@tomviner
Copy link
Contributor

@The-Compiler see what you mean. So really, for patches that alter the pytest interface, only changes that were obviously broken before should be aimed at master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants