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

bpo-36654: add example to generate token from another file #12947

Merged
merged 4 commits into from Jan 25, 2020
Merged

bpo-36654: add example to generate token from another file #12947

merged 4 commits into from Jan 25, 2020
+19 −0

Conversation

@Windsooon
Copy link
Contributor

Windsooon commented Apr 25, 2019 •
edited by bedevere-bot

Add example to generate token from another file.

https://bugs.python.org/issue36654
@Windsooon
bpo-36654: add example to generate token from another file
Loading status checks…
77f1024
@eamanu
eamanu reviewed Apr 27, 2019
View changes
Copy link
Contributor

eamanu left a comment

This example confuse me, because the file example.py does not exist. Maybe you need to do something like hello.py
@berkerpeksag
berkerpeksag requested changes Apr 27, 2019
View changes
Doc/library/tokenize.rst Outdated Show resolved Hide resolved
Doc/library/tokenize.rst Outdated
Example of tokenizing from another file::

import tokenize
f = open('example.py', 'rb')

This comment has been minimized.

Sign in to view
Copy link
@berkerpeksag

berkerpeksag Apr 27, 2019

Member

Using tokenize.open() would be better as it automatically detect the encoding of the file:

cpython/Lib/tokenize.py

Line 449 in 5d90954

encoding, lines = detect_encoding(buffer.readline)

This comment has been minimized.

Sign in to view
Copy link
@berkerpeksag

berkerpeksag Apr 27, 2019

Member

Also, I agree with Emmanuel that using the existing hello.py as an example would make the example easier to follow.

This comment has been minimized.

Sign in to view
Copy link
@Windsooon

Windsooon Apr 28, 2019

Author Contributor

I found the tokenize function already call detect_encoding.

This comment has been minimized.

Sign in to view
Copy link
@berkerpeksag

berkerpeksag Apr 28, 2019

Member

I just realized that tokenize.open() returns a text stream, so passing it to tokenize.tokenize() wouldn't work.

We can use tokenize.generate_tokens() to demonstrate how the str API works:

import tokenize

with tokenize.open('hello.py') as f:
    token_gen = tokenize.generate_tokens(f.readline)
    for token in token_gen:
        print(token)

Then fallback to your example to show the usage of the bytes API:

import tokenize

with open('hello.py', 'rb') as f:
    token_gen = tokenize.tokenize(f.readline)
    for token in token_gen:
        print(token)
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Apr 27, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
@Windsooon
change example file name
Loading status checks…
dbafadc
@matrixise

This comment has been minimized.

Sign in to view
Copy link
Member

matrixise commented Sep 11, 2019

Hi @Windsooon

Could you update your PR with the last comment of @berkerpeksag and with the with statement?

Thank you
@matrixise
matrixise reviewed Sep 11, 2019
View changes
Copy link
Member

matrixise left a comment

Thanks for your contribution but you have to update your PR with the recommendations of @berkerpeksag
@Windsooon

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

Windsooon commented Sep 11, 2019

Hello, @matrixise, I would love to update the example if needed. However, Serhiy Storchaka didn't agree with it

I do not think a new example is needed. The existing example already demonstrates the use of file's readline method.

https://bugs.python.org/issue36654
@berkerpeksag

This comment has been minimized.

Sign in to view
Copy link
Member

berkerpeksag commented Sep 12, 2019

I think Serhiy's comment was about the current form of the PR. In my last comment, I was proposing to add examples for both bytes and unicode APIs of the tokenize module.

As a core developer, even I missed that generate_token() was made public. I'm pretty sure more people would like to know that the module now has an API to pass unicode input.
@csabella

This comment has been minimized.

Sign in to view
Copy link
Contributor

csabella commented Jan 12, 2020

@Windsooon, any updates? Thanks!
@Windsooon

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

Windsooon commented Jan 12, 2020

@Windsooon
add example for tokenizing file using unicode and bytes
Loading status checks…
f18f1e2
@Windsooon

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

Windsooon commented Jan 13, 2020

Hi, @csabella. I just updated the PR base on @berkerpeksag 's suggestion.
@csabella csabella requested a review from berkerpeksag Jan 13, 2020
@berkerpeksag
Update tokenize.rst
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
943c19d
@berkerpeksag
berkerpeksag approved these changes Jan 25, 2020
View changes
Copy link
Member

berkerpeksag left a comment

Looks pretty good to me, thank you!
@berkerpeksag berkerpeksag dismissed matrixise’s stale review Jan 25, 2020

Comments have been addressed.

@berkerpeksag berkerpeksag merged commit 4b09dc7 into python:master Jan 25, 2020
5 checks passed
5 checks passed
Docs
Details
Azure Pipelines PR #20200125.23 succeeded
Details
bedevere/issue-number Issue number 36654 found
Details
bedevere/news "skip news" label found
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 25, 2020

@berkerpeksag: Please replace # with GH- in the commit message next time. Thanks!
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Jan 25, 2020

Thanks @Windsooon for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖
miss-islington added a commit to miss-islington/cpython that referenced this pull request Jan 25, 2020
@Windsooon @miss-islington
bpo-36654: Add examples for using tokenize module programmically (pyt…
8725dc9 
…honGH-12947)

(cherry picked from commit 4b09dc7)

Co-authored-by: Windson yang <wiwindson@outlook.com>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 25, 2020

GH-18187 is a backport of this pull request to the 3.8 branch.
miss-islington added a commit to miss-islington/cpython that referenced this pull request Jan 25, 2020
@Windsooon @miss-islington
bpo-36654: Add examples for using tokenize module programmically (pyt…
d529008 
…honGH-12947)

(cherry picked from commit 4b09dc7)

Co-authored-by: Windson yang <wiwindson@outlook.com>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 25, 2020

GH-18188 is a backport of this pull request to the 3.7 branch.
berkerpeksag added a commit that referenced this pull request Jan 25, 2020
@miss-islington @Windsooon @berkerpeksag
bpo-36654: Add examples for using tokenize module programmatically (G…
Loading status checks…
6dbd843 
…H-12947)

(cherry picked from commit 4b09dc7)

Co-authored-by: Windson yang <wiwindson@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eamanu eamanu

@berkerpeksag berkerpeksag

@matrixise matrixise

Assignees
No one assigned
Labels
CLA signed skip news type-documentation
Projects
None yet
Milestone
No milestone
8 participants
@Windsooon @bedevere-bot @matrixise @berkerpeksag @csabella @miss-islington @eamanu @the-knights-who-say-ni
You can’t perform that action at this time.