Skip to content

Refactor lexer functions #4288

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

Merged
merged 3 commits into from
Nov 24, 2022
Merged

Refactor lexer functions #4288

merged 3 commits into from
Nov 24, 2022

Conversation

yt2b
Copy link
Contributor

@yt2b yt2b commented Nov 22, 2022

I refactored following lexer functions.

  • lex_number
  • take_number
  • at_exponent
  • is_identifier_continuation

What I have done are as follows.

  • using match expression instead of if expression
  • reducing the nesting of match expression

@youknowone
Copy link
Member

youknowone commented Nov 22, 2022

@bonjune could you please review this PR?

@bongjunj
Copy link
Contributor

@bonjune could you please review this PR?

I'll look into it today.

Copy link
Contributor

@bongjunj bongjunj left a comment

Choose a reason for hiding this comment

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

I like how the pattern matching here improves readability.
But, as I left a comment, if expression should be preferred for boolean values.

@yt2b
Copy link
Contributor Author

yt2b commented Nov 23, 2022

@bonjune
Code fix completed.

Copy link
Contributor

@bongjunj bongjunj left a comment

Choose a reason for hiding this comment

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

Good!
Have a good day. Thank you.

@yt2b
Copy link
Contributor Author

yt2b commented Nov 24, 2022

@bonjune
Could you please review again?

Copy link
Contributor

@bongjunj bongjunj left a comment

Choose a reason for hiding this comment

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

Looks good.

P.S.

https://doc.rust-lang.org/std/primitive.bool.html#method.then_some

I read bool's two method implementation: bool::then and bool::then_some, whose behaviors are subtly different. The key thing is here: the former lazily evaluated the passed closure while the latter one evaluates passed value eagerly. In this case, bool::then is a proper choice because we don't to evaluate the function only when take_char is true. (And you made the right decision, I think)

cc @youknowone

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!

@youknowone youknowone merged commit 00ba73d into RustPython:main Nov 24, 2022
@yt2b yt2b deleted the refactor_lexer branch November 24, 2022 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants