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

Make all variables class instance variables #86

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

alan412
Copy link
Collaborator

@alan412 alan412 commented Mar 2, 2025

Fixes #29

This was the simplest way I could come up with to make all variables part of the class (using self.). It allows us to use all the normal built-in blockly things instead of needing to duplicate them.

@alan412 alan412 requested a review from lizlooney March 2, 2025 21:00
Copy link
Collaborator

@lizlooney lizlooney left a comment

Choose a reason for hiding this comment

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

When testing, I noticed that the python code still has the "self.variable = None" statements before the actual assignments.

Screenshot 2025-03-03 at 9 07 19 PM

Is that temporary?

I approve this PR, but please address my review comments (which I think are all about the comments in the code) before merging it.

@alan412
Copy link
Collaborator Author

alan412 commented Mar 5, 2025

When testing, I noticed that the python code still has the "self.variable = None" statements before the actual assignments.

Is that temporary?
I did that because it was how Blockly did it, but now that I think about it the reason they had to do that was so that the variables would have global scope. I don't think we have that problem, so I'll get rid of it.

@alan412 alan412 merged commit 178dc39 into wpilibsuite:main Mar 5, 2025
1 check passed
@alan412 alan412 deleted the pr_class_instance_variables branch March 5, 2025 00:55
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.

Add class name variable support
2 participants