Skip to content
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

added python version to parse error message #3323

Closed
wants to merge 8 commits into from

Conversation

nnrepos
Copy link
Contributor

@nnrepos nnrepos commented Oct 10, 2022

Description

improve parsing error message to make sure users know which version it was run on (fixes #3294).

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

modified `CHANGES.md`
modified other relevant markdown files
modified relevant tests
@github-actions
Copy link

github-actions bot commented Oct 10, 2022

diff-shades reports zero changes comparing this PR (fc39bca) to main (d338de7).


What is this? | Workflow run | diff-shades documentation

@ichard26 ichard26 self-requested a review October 10, 2022 16:17
@felix-hilden felix-hilden added skip news Pull requests that don't need a changelog entry. and removed skip news Pull requests that don't need a changelog entry. labels Oct 15, 2022
Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this, look good apart from a changelog detail 👍

CHANGES.md Outdated Show resolved Hide resolved
Co-authored-by: Felix Hildén <[email protected]>
@nnrepos nnrepos requested review from felix-hilden and removed request for ichard26 October 20, 2022 17:45
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I feel like this will create more user confusion:

% black --target-version py36  -c 'a++'
a++
error: cannot format <string>: Cannot parse (python 3.0): 1:3: a++
% black --target-version py39  -c 'a++'
a++
error: cannot format <string>: Cannot parse (python 3.7): 1:3: a++

Users are going to wonder why it says "3.0" or "3.7" when they never asked for that version.

CHANGES.md Outdated Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <[email protected]>
@nnrepos
Copy link
Contributor Author

nnrepos commented Oct 26, 2022

I feel like this will create more user confusion:

% black --target-version py36  -c 'a++'
a++
error: cannot format <string>: Cannot parse (python 3.0): 1:3: a++
% black --target-version py39  -c 'a++'
a++
error: cannot format <string>: Cannot parse (python 3.7): 1:3: a++

Users are going to wonder why it says "3.0" or "3.7" when they never asked for that version.

ok, can you help me understand why we parse python 3.0, when a user asks to parse py36?

@JelleZijlstra
Copy link
Collaborator

ok, can you help me understand why we parse python 3.0, when a user asks to parse py36?

We don't have separate grammars for every version. For py36 we use a grammar with version set to (3, 0).

@nnrepos
Copy link
Contributor Author

nnrepos commented Oct 26, 2022

ok, can you help me understand why we parse python 3.0, when a user asks to parse py36?

We don't have separate grammars for every version. For py36 we use a grammar with version set to (3, 0).

ok, so do we have an access to the version that the user provided somewhere?

@JelleZijlstra
Copy link
Collaborator

It's the target version, which gets passed as a command-line parameter and thence as an argument to main().

@nnrepos nnrepos requested review from JelleZijlstra and removed request for felix-hilden October 27, 2022 22:04
@nnrepos
Copy link
Contributor Author

nnrepos commented Nov 4, 2022

hey @JelleZijlstra @felix-hilden, i've made some changes, please let me know if this is helpful 😄

@JelleZijlstra
Copy link
Collaborator

I'm still not enthusiastic about this:

  • Hardcoding the list of versions is a little hacky and hard to maintain
  • I'm afraid the error message may still be confusing for users, but don't have a better suggestion

@nnrepos
Copy link
Contributor Author

nnrepos commented Nov 7, 2022

I'm still not enthusiastic about this:

  • Hardcoding the list of versions is a little hacky and hard to maintain
  • I'm afraid the error message may still be confusing for users, but don't have a better suggestion
  • i've used the same hard-coding that is used by the parser, since there weren't any constants/functions which can be used to do that. that can be refactored of course.
  • i think users are smart enough to understand what a range of numbers is, we're all programmers here...

feel free to close this if you're not interested

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.

Clearer errorr message for "Cannot parse" when targetting multiple version
3 participants