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: Fixes and improved clarity for unique-name behavior #137

Merged
merged 9 commits into from
Jun 9, 2024

Conversation

towerofnix
Copy link
Member

@towerofnix towerofnix commented May 24, 2024

Development:

Addresses the stuff above! Notable details:

  • We've moved these lists, still hard-coded, into top-level constants, rather than embedding them right inside the toLeopard function. This makes them more convenient to locate and edit, and separates particularly word-based data from actual conversion logic.
  • We've updated and added new documentation strings to relevant top-level constants. These hopefully offer some improved clarity — particularly useful since these are important lists to keep maintained!
  • We've renamed uniqueNameGenerator to uniqueNameFactory because generator functions and factory functions are different things, and this is the latter. (Best not to conflate with the JavaScript term "generators", especially since Leopard itself makes extensive use of JS generators.)
  • We've improved some internal spot documentation. This probably belongs in the next PR, but it's just better describing the current state of the code, so...

Here's the big external change, besides the word-list fixes listed at the start:

  • We're using separate reserved word lists, based on a common list, for the stage and sprites now. We've closely reviewed the current Leopard definitions of SpriteBase, Stage, and Sprite to make sure we're covering everything (and not leaving some crucial things only in the sprite list and not the stage list, for example). But a second pair of eyes comparing that code with this would be great, of course!

We have also introduced a new JS_RESERVED_PROPERTIES list. These are names you really don't want generated code to ever be messing with! This is mostly to cover our bases when we do variables on this. We technically won't ever hit __proto__ because that's not a camel-case name, and prototype is only meaningful on functions (which sprite instances aren't)... but constructor really does fix a bug (#136), and we figured better to be a little more comprehensive than less, in such a context-generic list.

@towerofnix towerofnix added bug Something isn't working documentation Improvements or additions to documentation fmt: Leopard Pertains to Leopard format (JavaScript) labels May 24, 2024
@towerofnix towerofnix requested a review from PullJosh May 24, 2024 00:19
const LEOPARD_RESERVED_SPRITE_NAMES = [
// Browser-provided identifiers
"Date",
"Math",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Do we want to blacklist browser-provided identifiers like Array, Audio, Error, File, Map, Set, Temporal, etc., which are not used in generated Leopard code but could cause conflicts or confusion when the user starts adding their own hand-written JS code?

There are a lot of them. And keeping up-to-date might be challenging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this is a good point! We've followed it up in detail here: #138

src/io/leopard/toLeopard.ts Outdated Show resolved Hide resolved
@PullJosh
Copy link
Collaborator

PullJosh commented May 25, 2024

This looks nice! Thanks. 😄

I haven't properly tested the behavior, but the code looks good (aside from one comment with a requested fix). If you're reasonably confident that the behavior is correct, I'm happy with merging this and then maybe I can give it a proper test of my own after switching away from this.vars.* before publishing a release.

Or if you'd rather wait until I can really sit down with it, it'll just take a few days.

@towerofnix
Copy link
Member Author

Oops! Thanks for that catch LOL.

We'd prefer that you take the time to check it out before we merge. It fixes some bugs and otherwise is a totally non-breaking release, so we'd kind of like to publish it as a separate version from the change for this.vars.* -> this.*. Of course we can work out any quirks while testing in general, and the current version will only be useful historically speaking, but it'd still be nice to have this validated and published as its own thing. (The last bastion of this.vars.* syntax, lol.)

No problem about the delay, by the way. Thanks to your blessing/look-over we can say this is a good base, so we can get started on this.vars.* -> this.* in a separate branch sooner, just starting from these commits.

@towerofnix
Copy link
Member Author

Addendum for this PR: While we can generate the lists dynamically and certainly include a broader list of globals than just what sb-edit depends on accessing—we are still OK to start on implementations based on these top-level constants. Because, we'd just be changing how the lists are generated, not how they are externally accessed (still the same variable names that still represent the same practical contents, just better versions of those lists).

@PullJosh
Copy link
Collaborator

PullJosh commented Jun 9, 2024

Tested and looks good! 😄 (Sorry for the delay)

@PullJosh PullJosh merged commit 479b884 into leopard-js:master Jun 9, 2024
1 check passed
@towerofnix towerofnix deleted the nicer-unique branch June 9, 2024 21:24
@towerofnix
Copy link
Member Author

towerofnix commented Jun 9, 2024

No problem! We've been busy anyway LOL. 🥳 Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation fmt: Leopard Pertains to Leopard format (JavaScript)
Projects
None yet
2 participants