Skip to content

Turn --hide-error-context on by default #2540

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
merged 9 commits into from
Dec 9, 2016
Merged

Conversation

ddfisher
Copy link
Collaborator

@ddfisher ddfisher commented Dec 9, 2016

This diff provides a flag called --show-error-context in place of --hide-error-context and flips the default.

This also removes all context lines from tests so they do not need to be special cased. Here's why:

  • It's much easier to write tests when not having to worry about error context, so this is nicer going forward.
  • It avoids adding any more special casing to tests.
  • This won't results in any git blame problems because the transformation is purely subtractive.
  • This shouldn't be particularly difficult to review, because the transformations are split into separate commits.

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 9, 2016

This looks good to me. Just a few things:

  • Let's also give @gvanrossum (and others) a chance to comment for a while.
  • Add test for specifying the equivalent of --show-error-context in the config file (assuming it's supported; if not, we should add this option).
  • Maybe add a short command line option for enabling error context, since it can sometimes be useful and writing --show-error-context is kind of inconvenient.

@gvanrossum
Copy link
Member

I recommend keeping the --hide-error-context flag, for backward compatibility reasons, but with help=argparse.SUPPRESS. This allows enabling it from the command line when disabled from mypy.ini, and avoids breaking scripts that pass the old flag. Otherwise LG. (I don't feel the need for a short option.)

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 9, 2016

OK, we can leave out the short option. It will be easy enough to add in the future in case it feels like it would be useful to have.

@gvanrossum
Copy link
Member

I'm merging this now while it has no merge conflicts yet. I'll open a new issue for keeping the old option.

@gvanrossum gvanrossum merged commit 5395d78 into master Dec 9, 2016
@gvanrossum gvanrossum deleted the hide-error-context branch December 9, 2016 19:09
gvanrossum pushed a commit that referenced this pull request Dec 9, 2016
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.

None yet

3 participants