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

Merge Prettified Corax+ Code #24

Merged
merged 3 commits into from
Aug 17, 2024
Merged

Merge Prettified Corax+ Code #24

merged 3 commits into from
Aug 17, 2024

Conversation

GamingMadster
Copy link
Contributor

Here's the code cleanup I mentioned doing in #22.

  • Added spacing for the code so the code was a bit easier to read through
  • Removed unnecessary comments, added a couple others
  • Made labels a bit more consistent throughout

I might try and do the same for some of the other source files, just so it's easier for someone looking to contribute to be able to read through the code.

@Timendus
Copy link
Owner

The Corax+ test is the worst offender of the bunch, I think. Primarily because I've initially tried to keep it just the "stock" Corax89 ROM with a few patches / bug fixes. I've added more and more changes over the years, without ever really taking the time to refactor.

I think your PR is an improvement overall, couple of remarks:

  • Your version is removing a newline at the end of the file. I'm guessing your editor is configured to remove it. Just like the tabs/spaces discussion, this is a very subjective thing, and I generally just conform to whatever the rest of the project I'm contributing to uses.
  • I like adding whitespace for readability. I'll go through later to see if I agree with your groupings.
  • I don't see any changes to labels..?

All in all: congratulations on a much cleaner and much easier to review pull request than the last one! Great to see you learning in real time 😄

@GamingMadster
Copy link
Contributor Author

GamingMadster commented Aug 15, 2024

To be honest, combing through the code wasn't all too bad, but some areas were sure a doosie.

  • In terms of the labels, I think I'm combining the labels and comments for the start of each test in my mind, oops. 😅
    I do think I still modified atleast one though?

  • The whitespace i mainly did for the if statements that set i to image-no, although there are a couple other areas that I did it upon which you may deem unnecessary, unsure.

  • In terms of the newlines disappearing, I've just been using notepad to get the job done. I don't know why it would remove the newline at the end, since it's a simple text editor and it shouldn't auto format it. I guess I might have accidentally deleted it, oops x2. 😅

Oh, and thanks! Again, I'm glad to be contributing to the test suite!

i := image-no
: test1x

: test-1X-pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, yeah, I knew I wasn't going crazy. This was the only label I changed. 😄

@GamingMadster
Copy link
Contributor Author

GamingMadster commented Aug 15, 2024

Alright, I've done a little bit more to cleanup stuff.

  • Did some small comment changes
  • Added a few comments of my own
  • Hopefully re-added the newline? (I just copied the line needed from the file before my commits ¯\_(ツ)_/¯)

Comment on lines 46 to 49
# Note by Madster: I have seen one interpreter, of which I've forgotten the
# name of, that allows the execution of the opcode succeeding the return. There
# is a chance that this would fail on that interpreter; however, I'll need to dig up that
# interpreter to test that, though.
Copy link
Owner

Choose a reason for hiding this comment

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

Then that interpreter would be broken, and this test would successfully catch that issue ;)

@Timendus
Copy link
Owner

Thanks! I've removed many of the newlines again :P Sorry for that, but I do think it's a lot more readable now. Thanks for your help :)

@Timendus Timendus merged commit 88c29ea into Timendus:main Aug 17, 2024
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