Skip to content

/ cpython

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

Jump to bottom

bpo-40176: Improve error messages for unclosed string literals #19346

Open
wants to merge 3 commits into
base: master
from
Open

bpo-40176: Improve error messages for unclosed string literals #19346

wants to merge 3 commits into from
+26 −32

Conversation

@isidentical
Copy link
Member

@isidentical isidentical commented Apr 3, 2020 •
edited by bedevere-bot

https://bugs.python.org/issue40176
@ammaraskar
Copy link
Member

@ammaraskar ammaraskar commented Apr 3, 2020

This message is still a bit jargon-y, I think Rust's error message in this case is pretty nice:

error: unterminated double quote string
 --> src/main.rs:2:19
  |
2 |       let message = "Hello world
  |  ___________________^
3 | |     println!(message);
4 | | }
  | |_^
@ammaraskar ammaraskar requested a review from benjaminp Apr 3, 2020
@isidentical isidentical force-pushed the isidentical:bpo-40176 branch from 9294171 to 50f6de2 Apr 3, 2020
@isidentical
Copy link
Member Author

@isidentical isidentical commented Apr 3, 2020

This message is still a bit jargon-y, I think Rust's error message in this case is pretty nice:

That definitely looks more simpler to those who unfamiliar with parser jargon. I'm not sure if we should still use EOF/EOL when needed or not though.
@isidentical isidentical force-pushed the isidentical:bpo-40176 branch 4 times, most recently from c75e26e to 0b6386d Apr 3, 2020
@isidentical isidentical requested a review from serhiy-storchaka Apr 4, 2020
@SylvainDe
SylvainDe reviewed Apr 26, 2020
View changes
Lib/test/test_eof.py
self.assertEqual(str(msg), expect)
self.assertEqual(msg.offset, 1)
else:
raise support.TestFailed

def test_EOFS(self):

This comment has been minimized.

Sign in to view
@SylvainDe

SylvainDe Apr 26, 2020
Contributor

If the symbol "EOFS" is removed from the code, it may be worth changing the test name for something more explicit.

Out of curiosity, would it make sense to perform the test for both types of quotes like you did in the other test ?
@isidentical isidentical force-pushed the isidentical:bpo-40176 branch from c7b3048 to 4c24faa Jan 20, 2021
isidentical added 2 commits Jan 20, 2021
@isidentical
bpo-40176: point to string opening on EOF/EOL
10bdaa8
@isidentical
revert the lineno pointer back to the string start
Loading status checks…
fcecf4b
@isidentical isidentical force-pushed the isidentical:bpo-40176 branch from 4c24faa to fcecf4b Jan 20, 2021
@isidentical
add blurb entry
Loading status checks…
b52364e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SylvainDe SylvainDe

@benjaminp benjaminp

@serhiy-storchaka serhiy-storchaka

@pablogsal pablogsal

@lysnikolaou lysnikolaou

Assignees
No one assigned
Labels
CLA signed awaiting review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@isidentical @ammaraskar @SylvainDe @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.