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.

Sign up for GitHub

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

Improve string fstring coverage #4231

Merged
merged 5 commits into from
Oct 27, 2022
Merged

Improve string fstring coverage #4231

merged 5 commits into from
Oct 27, 2022

Conversation

dvermd
Copy link
Contributor

@dvermd dvermd commented Oct 17, 2022

The test_fstring.py file comes from CPython

This improve coverage as of #1671.

@dvermd dvermd force-pushed the improve_string_fstring_coverage branch from 4202468 to fdc5746 Compare October 17, 2022 20:25
@dvermd dvermd marked this pull request as draft October 17, 2022 20:39
@dvermd dvermd force-pushed the improve_string_fstring_coverage branch 3 times, most recently from 0871a91 to afcfa62 Compare October 18, 2022 06:48
@youknowone
Copy link
Member

@charliermarsh could you review this PR when it is ready?

@fanninpm
Copy link
Contributor

There are currently a bunch of unexpectedly passing tests in test_ast.py and test_tokenize.py.

@dvermd dvermd marked this pull request as ready for review October 19, 2022 15:55
@dvermd
Copy link
Contributor Author

dvermd commented Oct 19, 2022

The change on python.lalrpop might be extended to all binop. What do you think ?

@dvermd dvermd force-pushed the improve_string_fstring_coverage branch from 1b4db38 to c00f9b0 Compare October 20, 2022 04:26
youknowone
youknowone reviewed Oct 21, 2022
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you for contributing. I left a few comments about policy and styles.

@charliermarsh Could you please do the technical review?

Lib/test/test_global.py Outdated
@@ -25,15 +25,15 @@ def wrong1():
global a
global b
"""
check_syntax_error(self, prog_text_1, lineno=4, offset=5)
check_syntax_error(self, prog_text_1, lineno=4, offset=4)
Copy link
Member

Choose a reason for hiding this comment

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

We mostly try not to make changes in cpython-originated code.
Please keep this as original code and add @unittest.expectedFailure if you cannot keep this compatibility with the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

@dvermd dvermd Oct 24, 2022 •
edited

Choose a reason for hiding this comment

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

I think this change might be handle the same way this CPython commit handles it 025eb98dc0: make all syntax error 1-based by incrementing location in "constructor" by 1.

That should go into another issue, don't you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, thank you! good to know.

compiler/core/src/location.rs Outdated Show resolved Hide resolved
compiler/parser/src/fstring.rs Outdated Show resolved Hide resolved
@dvermd dvermd force-pushed the improve_string_fstring_coverage branch from 5c8de04 to 739d15a Compare October 24, 2022 05:13
@fanninpm
Copy link
Contributor

Can you split d34d361 into two separate commits: one commit adding the test file, and one commit adding the RustPython changes?

@charliermarsh
Copy link
Collaborator

(I promise to review when I can, if others get to it first no prob.)

@dvermd dvermd force-pushed the improve_string_fstring_coverage branch from 739d15a to 8787a52 Compare October 24, 2022 15:06
@dvermd
Copy link
Contributor Author

dvermd commented Oct 24, 2022

Can you split d34d361 into two separate commits: one commit adding the test file, and one commit adding the RustPython changes?

Done

@youknowone youknowone mentioned this pull request Oct 24, 2022
Closed
@dvermd
Copy link
Contributor Author

dvermd commented Oct 24, 2022

Looks like some more unexpected success appeared

@dvermd dvermd force-pushed the improve_string_fstring_coverage branch from 64952ae to 24ee353 Compare October 24, 2022 20:37
@youknowone
Copy link
Member

which version of CPython did you use for d48d998 ?

@youknowone youknowone mentioned this pull request Oct 26, 2022
Merged
@youknowone youknowone force-pushed the improve_string_fstring_coverage branch 5 times, most recently from 2c821e4 to cbcfb9f Compare October 26, 2022 18:45
charliermarsh
charliermarsh reviewed Oct 26, 2022
View reviewed changes
compiler/parser/src/fstring.rs Outdated Show resolved Hide resolved
compiler/parser/src/fstring.rs Outdated Show resolved Hide resolved
compiler/core/src/location.rs Outdated Show resolved Hide resolved
@dvermd dvermd force-pushed the improve_string_fstring_coverage branch from cbcfb9f to def4da5 Compare October 26, 2022 19:16
@dvermd
Copy link
Contributor Author

dvermd commented Oct 26, 2022

which version of CPython did you use for d48d998 ?

I took it from CPython 3.10.8

@fanninpm
Copy link
Contributor

which version of CPython did you use for d48d998 ?

I took it from CPython 3.10.8

I think @youknowone wants you to amend the commit name to mention that version.

@dvermd
Copy link
Contributor Author

dvermd commented Oct 26, 2022

Is it intended that all locations are now 0-indexed rather than 1-indexed?

@charliermarsh It looks like it's how CPython handles it regarding how it improve coverage in test_ast.py.

Also @youknowone already merged this part of the PR.

@dvermd dvermd force-pushed the improve_string_fstring_coverage branch from def4da5 to af04e3e Compare October 26, 2022 19:31
@dvermd
Copy link
Contributor Author

dvermd commented Oct 26, 2022

which version of CPython did you use for d48d998 ?

I took it from CPython 3.10.8

I think @youknowone wants you to amend the commit name to mention that version.

commit amended with the CPython version

youknowone
youknowone approved these changes Oct 27, 2022
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

@dvermd Thank you! This is a big progress!

@charliermarsh @fanninpm Thank you for the reviews!

@youknowone youknowone merged commit 426f9f6 into RustPython:main Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charliermarsh charliermarsh charliermarsh left review comments

@fanninpm fanninpm fanninpm left review comments

@youknowone youknowone youknowone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dvermd @youknowone @fanninpm @charliermarsh