Skip to content

Vjp/refactor player pt2 #10

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 6 commits into from
Feb 22, 2022
Merged

Vjp/refactor player pt2 #10

merged 6 commits into from
Feb 22, 2022

Conversation

vanceism7
Copy link
Collaborator

@vanceism7 vanceism7 commented Jan 13, 2022

This PR factors all the global variable setting code into its own function, and then breaks that function down into a number of sub functions just to clean it up a little.

You can review this commit by commit if you wanna see the individual changes. It's not too crazy though

Something im curious about:
In the function adjust_operands, we have this line

num,ct = peel_uint(val[:ct])

It looks like these are local variables but weren't being used anywhere. If they're used in some broader context, then perhaps factoring the handle_assign function could've broke something? In testing, all of the % commands still seem to be working, but this might need some fixing up if not.

Vance Palacio added 5 commits January 10, 2022 21:14
A lot of this is just shuffling stuff around but it does make the code a
bit more self-documenting. handleAssign could probably be factored
further but we don't want this to get too out of hand..
@vanceism7 vanceism7 requested a review from flipcoder January 13, 2022 01:45
@flipcoder
Copy link
Owner

Yeah there's definitely some problems with my parser code here. It also looked like there's an undefined count under one of the conditions that probably wasn't tested. I'll get it cleaned up

@flipcoder flipcoder merged commit c7c6f1a into develop Feb 22, 2022
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