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.

Sign up for GitHub

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

Jump to bottom

Refactor and bug fix #60

Merged
merged 70 commits into from
Nov 30, 2022
Merged

Refactor and bug fix #60

merged 70 commits into from
Nov 30, 2022

Conversation

Rudxain
Copy link
Member

@Rudxain Rudxain commented Oct 5, 2022 •
edited
Loading

  • Fixed unresolved import (commit link)
  • Added helpers.py for easier code sharing between modules
  • Convert magic numbers/strings into enums to get the benefits from vars and dicts
  • Added more type annotations for static type checking
  • Added Final annotations to avoid accidental assignment of immutable vars (also to reduce cognitive load of managing global vars)
  • Added and edited comments (regular and docs)
  • Make 0Byte programs valid (doesn't work with crickroll)
  • etc...

Disclaimer: I tested most (not all) of the changes

@Rudxain Rudxain changed the title Some refactoring Refactoring and bug fix Oct 7, 2022
@SatinWukerORIG
Copy link
Member

Wow those are a lot of changes.

@Rudxain
Add helpers.py
91c993b 
This way we have a centralized module from which to `import` common functionality
@Rudxain
Update pyrickroll.py
da1919e
@Rudxain
fmt
4797ead
@Rudxain
move filter_str & precedence to helpers.py
c059215 
Also add more type annotations
@Rudxain
Copy link
Member Author

Rudxain commented Oct 8, 2022 •
edited
Loading

Wow those are a lot of changes.

Yeah lol. I'm almost done (I guess). Don't worry, I'll try to keep the diff small (for easier review)

@Rudxain
Copy link
Member Author

Rudxain commented Oct 8, 2022

I realized using a keyword dictionary instead of an enum is a bad idea, because it doesn't have the benefits of vars. IDEs can easily catch enum typos, but not dict-key typos, and it also allows for auto-completion

@Rudxain Rudxain marked this pull request as ready for review October 8, 2022 03:51
@SatinWukerORIG
Copy link
Member

You are right lol. I did not put effort in code optimization, sorry for causing this inconvenience. Thank you for making these changes, I appreciate your hard work!

@Rudxain
Remove unused dict_values
e20213a
@Rudxain
More "polite" (IDK?) comment in Keywords.py
bca7dbb 
I wrote the comment as "imperative", but I changed it to "explanatory", to be a note targeted to everyone (code-owners and contributors alike)
@Rudxain
Rename remove_all params
d135a20
@Rudxain
Forgot some stuff lol
ea81af9 
Fixed a `KW.GOE_OP` where a `KW.G_OP` should be. Added some more type annotations. Formatted some stuff. Moved a `dict` assignment outside of a branch (for performance, and potential reuse in the future)
@SatinWukerORIG
Copy link
Member

Yes, it is better if you finish everything in one PR lol, and after this change, I will merge it.

@Rudxain
Copy link
Member Author

Rudxain commented Oct 21, 2022

Ok lol. I'm gonna speedrun this! (it's a joke, don't worry lol, I'll take my time to avoid more mistakes)

@Rudxain Rudxain marked this pull request as draft October 21, 2022 18:15
@Rudxain
Final in AudioGenerator.py &
d632d71
@Rudxain
comment explaining True if a == b else False
ecade91
@Rudxain
mode comment to helpers
ec30477
@Rudxain
rename mutables to lowercase, so Pylance stops shouting at me, lol
56f6128
@Rudxain
Update crickroll.py
4a6ea81
@Rudxain
local imports, applyOp arg types, accurate time
c4cc314
@Rudxain
format
6f10ece
@Rudxain
fix 0B bug in play_audio, simplify for player
c211382
@Rudxain
convert Token Types to an enum
3c01727
@Rudxain
convert some regular comments to docs
e23629f
@Rudxain
doc-comments
944eb3a
@Rudxain
def to lambda, set to tuple
f776a18
@Rudxain
minify c_separators
ccdcef9
@Rudxain
rename all enums to uppercase
7382e58
@Rudxain
Final
8efd4c3
@Rudxain
Delete settings.json
028d8bb
@Rudxain
Keywords.py -> uppercase, OP_build_in_functions -> `OP_BUILT_IN_FUN…
56b0265 
…CTIONS`
@Rudxain
consistent quotes in crickroll
5cd031e
@Rudxain
Update Keywords.py
0aaf72f
@Rudxain
avoid name collision
26d9d64
@Rudxain
final commit?
753f1f7 
I guess
@Rudxain Rudxain marked this pull request as ready for review November 24, 2022 00:08
@Rudxain
Copy link
Member Author

Rudxain commented Nov 24, 2022 •
edited
Loading

@SatinWuker it's ready! Please give me feedback, there might be places where it may be better to revert changes (like making a type annotation less verbose, or formatting in a different way, etc...). There may be bugs/mistakes, but I "triple-checked", so it's unlikely, lol

@SatinWukerORIG
Copy link
Member

SatinWukerORIG commented Nov 30, 2022 •
edited
Loading

Thank you! Those are some huge changes lol, I am merging it.

@SatinWukerORIG SatinWukerORIG merged commit cfce12c into Rick-Lang:main Nov 30, 2022
@Rudxain Rudxain deleted the patch-1 branch November 30, 2022 02:09
@Rudxain Rudxain mentioned this pull request Dec 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rudxain @SatinWukerORIG @FusionSid