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

bpo-38870: Simplify tuple like interleaves, roundtrip extslice properly #17892

Open
wants to merge 4 commits into
base: master
from

Conversation

@isidentical
Copy link
Contributor

isidentical commented Jan 7, 2020

Lib/ast.py Outdated Show resolved Hide resolved
@isidentical isidentical requested a review from pablogsal Jan 7, 2020
Lib/ast.py Outdated Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
Lib/ast.py Outdated Show resolved Hide resolved
@pablogsal pablogsal added the skip news label Jan 7, 2020
Co-Authored-By: Pablo Galindo <Pablogsal@gmail.com>
@isidentical

This comment has been minimized.

Copy link
Contributor Author

isidentical commented Jan 7, 2020

Thanks for the suggestions @pablogsal!

@isidentical isidentical requested a review from pablogsal Jan 7, 2020
self.write(",")
else:
self.interleave(lambda: self.write(", "), self._write_constant, value)
self.items_view(self._write_constant, value)

This comment has been minimized.

Copy link
@pablogsal

pablogsal Jan 7, 2020

Member

There are no test for these cases (tuples), aren't they?

This comment has been minimized.

Copy link
@isidentical

isidentical Jan 7, 2020

Author Contributor

as I stated in the issue, no. We can't test constant tuples with the classic way so I am planning to add a test after #17377 or #17760 (they both add a way to test generated source with given node instead of ast checks).

This comment has been minimized.

Copy link
@pablogsal

pablogsal Jan 7, 2020

Member

as I stated in the issue, no

Then shouldn't we add that code once the test can be added?

This comment has been minimized.

Copy link
@isidentical

isidentical Jan 7, 2020

Author Contributor

We already had some missing tests, including constant tuples.

This comment has been minimized.

Copy link
@pablogsal

pablogsal Jan 7, 2020

Member

We already had some missing tests, including constant tuples.

That does not justify adding even more missing test IMHO ;)

This comment has been minimized.

Copy link
@isidentical

isidentical Jan 7, 2020

Author Contributor

We already dont have tests for that,

Yes, unfortunately it isn't :/ Lets wait one of the PRs I mentioned before and we can add tests for constant tuples.

@pablogsal

This comment has been minimized.

Copy link
Member

pablogsal commented Jan 7, 2020

The build is failing because you have trailing whitespace (you can run make patchcheck to make the fixes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.