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

Update parse_tree_builder.py #1291

Closed
wants to merge 1 commit into from

Conversation

jmishra01
Copy link
Contributor

List function is not required, and rule check in callback dict should be after loop

@erezsh
Copy link
Member

erezsh commented Jul 3, 2023

Hello! Thanks for the PR. You're right about moving the rule check up in the loop. But I prefer that you avoid changing lines only for cosmetic reasons, especially when it's very subjective, like removing blank lines that separate code blocks.

List function is not required, and rule check in callback dict should be after loop

Update parse_tree_builder.py

Update parse_tree_builder.py

revoke line changes
@jmishra01
Copy link
Contributor Author

Revert changing lines changes.

@erezsh
Copy link
Member

erezsh commented Jul 5, 2023

@jmishra01 Thanks. Also, we're calling list() both times so that self.rule_builders can be permanent, which means create_callback() can be called more than once, and also that way it can be observed when debugging. It's not a huge difference, but there is just no reason not to do so. Sorry, I should have mentioned it in my previous message.

@jmishra01
Copy link
Contributor Author

@erezsh Thanks. Understood the reason behind calling list(). I'm closing this PR.

@jmishra01 jmishra01 closed this Jul 5, 2023
@jmishra01 jmishra01 deleted the list-not-required branch July 5, 2023 16:04
@erezsh
Copy link
Member

erezsh commented Jul 5, 2023

@jmishra01 You can re-open with just the rule-check. I'll be happy to merge that in.

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.

2 participants