GeezerVison

Consider not consuming all the buffer in one go in the tokenizer module #105069

Closed

pablogsal force-pushed the tokenizer_iter branch from ff4d45c to bc6d2da Compare

May 29, 2023 20:02

pablogsal mentioned this pull request

W391: spurious warnings with python 3.12 beta PyCQA/pycodestyle#1142

Closed

pablogsal force-pushed the tokenizer_iter branch 3 times, most recently from 0a661c4 to 713379f Compare

May 29, 2023 20:29

pablogsal marked this pull request as ready for review

pablogsal requested a review from lysnikolaou as a code owner

pablogsal force-pushed the tokenizer_iter branch from 713379f to 35328a4 Compare

pablogsal requested a review from lysnikolaou as a code owner

bedevere-bot added the awaiting core review label

pablogsal commented

View reviewed changes

Comment thread

Lib/test/test_tokenize.py

		def get_tokens(string):
		return list(_generate_tokens_from_c_tokenizer(string))

		self.assertRaises(SyntaxError, get_tokens, "(1+2]")

pablogsal May 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This was bothering me 😅

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal commented

View reviewed changes

Comment thread

Lib/tokenize.py

		This has the same API as tokenize(), except that it expects the readline
		callable to return str objects instead of bytes.
		"""
		def _gen():

pablogsal May 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Now that we are taking callables all of these can go :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal force-pushed the tokenizer_iter branch from 35328a4 to 1b8ff7e Compare

May 29, 2023 20:32


          pythongh-105069: Add a readline-like callable to the tokenizer to con…

7caac01

…sume input iteratively

Signed-off-by: Pablo Galindo <[email protected]>

pablogsal force-pushed the tokenizer_iter branch from 1b8ff7e to 7caac01 Compare

May 29, 2023 21:21

pablogsal added 3 commits

May 30, 2023 00:17


          fixup! pythongh-105069: Add a readline-like callable to the tokenizer…

8903d0d

… to consume input iteratively


          fixup! fixup! pythongh-105069: Add a readline-like callable to the to…

d370087

…kenizer to consume input iteratively


          fixup! fixup! fixup! pythongh-105069: Add a readline-like callable to…

2d6f0a6

… the tokenizer to consume input iteratively

pablogsal added the skip news label

pablogsal commented

View reviewed changes

Comment thread

Lib/inspect.py

		add(string)
		if (string == ','):
		add(' ')
		clean_signature = ''.join(text).strip()

pablogsal May 30, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This change is because for some reason the inspect module is relying on the fact that if lines yielded by the generator do not end in \n then they are concatenated together, which is wrong because the contract says "should yield one line at a time" so if the line doesn't end in newline we add one always.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

lysnikolaou reviewed

View reviewed changes

lysnikolaou left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Great job! 💯

A couple of comments and it's good to go!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Parser/tokenizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/tokenize.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

pablogsal added 2 commits

May 30, 2023 17:12


          fixup! fixup! fixup! fixup! pythongh-105069: Add a readline-like call…

…able to the tokenizer to consume input iteratively


          fixup! fixup! fixup! fixup! fixup! pythongh-105069: Add a readline-li…

9990b7e

…ke callable to the tokenizer to consume input iteratively

pablogsal commented May 30, 2023

Copy link

Copy Markdown

Member Author

Fixed the problems and added another test.

@lysnikolaou ready for another review!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pablogsal commented May 30, 2023

Copy link

Copy Markdown

Member Author

CC: @mgmacias95 wanna make a review?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          fixup! fixup! fixup! fixup! fixup! fixup! pythongh-105069: Add a read…

…line-like callable to the tokenizer to consume input iteratively

mgmacias95 approved these changes

View reviewed changes

mgmacias95 left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal merged commit 9216e69 into python:main

pablogsal deleted the tokenizer_iter branch

May 30, 2023 21:43

bedevere-bot removed the awaiting core review label

pablogsal added awaiting core review needs backport to 3.12 only security fixes labels

miss-islington commented May 30, 2023

Copy link

Copy Markdown

Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented May 30, 2023

Copy link

Copy Markdown

Contributor

Sorry @pablogsal, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 9216e69a87d16d871625721ed5a8aa302511f367 3.12

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington assigned pablogsal

pablogsal added needs backport to 3.12 only security fixes and removed needs backport to 3.12 only security fixes labels

miss-islington commented May 30, 2023

Copy link

Copy Markdown

Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          pythongh-105069: Add a readline-like callable to the tokenizer to con…

6de88f2

…sume input iteratively (pythonGH-105070)

(cherry picked from commit 9216e69)

Co-authored-by: Pablo Galindo Salgado <[email protected]>

bedevere-bot commented May 30, 2023

Copy link

Copy Markdown

GH-105119 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.12 only security fixes label