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

toLeopard: Code style cleanup for blockToJS & related functions #124

Merged
merged 5 commits into from
Dec 24, 2023

Conversation

towerofnix
Copy link
Member

@towerofnix towerofnix commented Dec 18, 2023

This pull request cleans up the code style contained by and surrounding blockToJS in a number of fairly high-impact ways, which together make it far easier to incrementally edit block definitions, compare one block to another, and generally read and understand the codebase. See individual commits; in summary:

  • Calls to inputToJS are basically never inlined as part of the blockSource template strings anymore; instead, they're defined in temporary variables, each on its own line, immediately above. This gets rid of the question of "is it worth it enough to store this input on a variable?" (always yes) and avoids some really brutal line breaking on particularly long template strings.
  • Every case OpCode.category_op now gets a proper block scope, i.e. case OpCode.foo: { ... }. This is to help with the new variables above, but also just makes the code style more consistent. (This pattern is reflected in other switch-cases in the same file, purely for consistency.)
  • Every case OpCode.category_op is divided into three sections: one which sets satisfiesInputShape, one which sets blockSource (including the related inputToJS calls), and one which is just the final break; at the end. This is mostly aesthetic, but it does lend a visual consistency across both those cases which are simpler and more complex.
  • Helper functions spriteInputToJS and colorInputToJS isolate commonly reused logic/patterns into helper functions. This doesn't actually change any behavior at all - all uses of spriteInputToJS followed exactly that behavior, etc. But it means there's a lot less logical volume being dedicated to this unnecessarily duplicate behavior, so block definitions are more focused on just what's unique about them.
  • Order of operations (i.e. deeper nested reporters being evaluated first) is enforced very simply by wrapping every instance of a reporter block in parentheses... as part of the layering function, inputToJS! This means we don't have to use loads and loads and loads of redundant parentheses, rather inconsistently, in block definitions. Accordingly, any explicitly written parentheses that remain (very few) actually carry significance, and don't get lost in the noise.
  • Some minor tweaks are made to code logic to clarify be a bit easier to parse, or more consistently written across similar block definitions. Some examples:
    • We always define let x and let y instead of let coords.
    • We always prefer switch/case when checking for one or more menu option values.
    • We always prefer checking typeof parseNumber(input) !== "number" instead of parseNumber(input) === null.
    • The logic for operator_add and operator_subtract is written with fewer nested cases and duplicate lines.
    • The section at the end of blockToJS which matches against desiredInputShape now uses a switch/case rather than a bunch of if-else cases, since it logically lines up with a switch/case/case/case/default quite well.

This PR basically doesn't change any behavior, apart from a few logic tidiness mentioned above, and the change to how order of operations is expressed. Since the latter has always been collaped by prettier, this doesn't affect sb-edit output at all — only how the code is expressed.

A neat way of reviewing this would probably be to view the diff in the "side by side" / split view appearance (rather than with additions and subtractions all in one column).

@towerofnix towerofnix added the fmt: Leopard Pertains to Leopard format (JavaScript) label Dec 18, 2023
Copy link
Collaborator

@PullJosh PullJosh left a comment

Choose a reason for hiding this comment

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

Looks great! As long as this passes all tests, I'm happy 👍

@towerofnix towerofnix merged commit 7c1af13 into leopard-js:master Dec 24, 2023
1 check passed
@towerofnix towerofnix deleted the leopard-cleanup branch December 24, 2023 03:01
@PullJosh
Copy link
Collaborator

There have been a lot of commits since a new release on npm. Is it time to do that soon?

@towerofnix
Copy link
Member Author

We haven't been in a rush for it since none of these are behavior changes, but the housekeeping stuff does clean up dependencies and update our name on the license - if you'd like to put a release online then everything is certainly good to go as-is.

#122 will probably be ready tomorrow, with the fix for #123 shortly after... but if pushing an extra NPM release is no hassle, then there's not much reason to wait pending those. ^^

@towerofnix
Copy link
Member Author

We may have forgotten that tomorrow is Christmas eve, LOL. Well, you know: those pull requests will be set when they're set, and that'll be sooner rather than later. Happy holidays! ✨ 📦

@PullJosh
Copy link
Collaborator

Lol. Maybe we wait for #122 and #123 because I don't remember the entire pushing process. Happy holidays! 📦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fmt: Leopard Pertains to Leopard format (JavaScript)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants