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

Deprecate sprite.vars.* in favor of using class properties directly? #196

Open
PullJosh opened this issue May 22, 2024 · 5 comments
Open
Labels
discussion Looking for feedback and input

Comments

@PullJosh
Copy link
Collaborator

Right now, Leopard targets (sprites and the stage) have a .vars object. Projects converted via sb-edit have all the Scratch variables converted to key/value pairs on the vars object.

When I edit Leopard projects manually, I am constantly annoyed by this and choose to just assign things directly to the sprite itself instead, like so:

// Current
this.vars.n = 5;

// What I prefer
this.n = 5;

This gives a far more "normal" JavaScript development experience, which I think is better for Leopard users who are learning to write JS code.

The only argument I can think of in favor of vars is that it provides a namespace so that you can, for example, have this.vars.x be a variable and this.x be the sprite's x-position. In my opinion, it would be better to stop using this namespace and instead have sb-edit convert to this.var_x and this.x or similar.

@PullJosh PullJosh added the discussion Looking for feedback and input label May 22, 2024
@towerofnix
Copy link
Member

Totally love it. Epic basketball score, lol.

this.vars is a nasty evil on Leopard as an educational tool and it just doesn't need to be there. It literally doesn't need to be there! You write code that uses this.n = 5 and it works! Gosh darn it!

I do think there are going to be awkwardnesses with project conversion that we... can't do a lot to avoid very prettily. I mean, the thing is that Leopard uses this.x to represent the sprite's position, whereas in Scratch that block is called "x position". So presumably, most Scratchers don't create their own variable named "x position", because that's awkward and hard to tell apart... they just make one named "x"!

I wonder if we can avoid using names like this._x. It's just awkward, and one (very small) character isn't enough to quickly tell this.x from this._x. Can we do this.xVar instead? It's not pretty, but it avoids the confusion, right?

Of course if sb-edit had some rather terrifying static analysis (leopard-js/sb-edit#98 (comment)) then we could just identify if a variable is only used within the function it converts into, and then just define it as a lexically bound variable with let. Buuuut it's okay to go without that and succumb to this._x or this.xVar or something similar, meanwhile, and when the case is necessary anyway.

@PullJosh
Copy link
Collaborator Author

Love it. We can bikeshed the naming collision situation as much as we want. I'm happy with this._x because it feels like it nicely denotes the "private" x that this sprite has as opposed to its public x coordinate. Or this.xVar or this.varX help users clearly understand which is the variable and which is the position.

And yes, terrifying static analysis is absolutely a good idea, once we build up the courage. Anything that can be a local variable should be a local variable. (Maybe at some point in the future we'll revise this statement, but I think it's at least mostly correct.)

@towerofnix
Copy link
Member

towerofnix commented May 25, 2024

I like the "private" thought, but I want to mention that it sort of conflicts with the this.sprites.MySprite._x syntax for the "(attribute/variable) of (sprite)" Sensing block. Maybe not fundamentally, but I do think there's a bit of an inconsistency if we don't name all for-this-sprite-only variables with an underscore. And that's just not JavaScript-y—JS has private class fields (i.e. private identifiers) to that end, and a sprite might expose that with get xPosed() { return this.#x }... (Bad pun...)

Incidentally, this._x is already a taken name by Leopard internals, but that's neither here nor there.

@PullJosh
Copy link
Collaborator Author

Alright, I think that's a good enough reason not to choose the underscore syntax.

Personally, between this.varX or this.xVar, I vote for varX. No clear reason why. I just find it easier to read in my brain. It helps me narrow in that first I'm thinking about a variable, and then that I'm thinking about the x variable specifically (kind of like how a YYYY/MM/DD format progressively narrows in a date). Although, to be honest, you could make the exact opposite argument as well, so it's really just my unfounded personal preference.

@towerofnix
Copy link
Member

OK, so we just wrote up leopard-js/sb-edit#138 (comment) and found that _x and _y are not in the list of names Leopard actually internally uses! So... clearly the TypeScript compiler and/or a minifier is optimizing those private names out. We would not be conflicting with them by using _x, _y etc. But yeah, it's still not very idiomatic, especially if other sprites read it.

Anytime verbiage could go one way or another and the world wouldn't end either way, we tend to not worry too badly about the outcome, LOL. Between this.varX and this.xVar we're happy either way.

  • varX is "nicer" and aligns with how we write our Scratch code (where all variables are sorted alphabetically, so generally, prefix-based notation is useful)...
  • ...although it's not necessarily common JS flavor (const kMaximumBananaVelocity = 413 is something you see in C++ code and in JS code written by C++ programmers).
  • xVar offers symmetry with ArraySprite...
  • ...and doesn't provide undue weight on prefix-based notation...
  • ...although Scratch likes prefixes, and your own code might have lots of prefixes...
  • ...although maybe we don't want to introduce external meaning in the same language space as your own prefixes...
  • ...but xVar does read more as an "afterthought", like, "um, no, I'm not talking about the innate property x, I'm talking about the variable xVar," which reflects the fact of the matter quite directly, and encourages you to rename the variable to something more specific...
  • ...but renaming exposed properties is pretty awkward in frankly all JavaScript, and certainly prone to bug-breeding if the user isn't very thorough in editing every place it is accessed...
  • ...but that goes for varX anyway, and frankly all variable renaming in a text-based programming language, c'est la vie. (Until your IDE, e.g. the Leopard editor, specifically aids you, but that's rarely a given.)
  • Also, your YYYY/MM/DD point is good and we like it, no matter if it's absolutely perfectly applicable or not.
    • Note that (the other way around) this.x and this.xVar look like they correspond to e.g. 2022 and 2022/11 respectively, but we would argue they closer correspond to 11 and 11/13 more closely. You can figure out that "11/13" means "the thirteenth day of November" no problem, but good luck telling if "11" on its own is a month or a day or even a year (did they just forget the apostrophe!? iMovie '11!?).

Bikeshedding is fun but also boring, so if you're conflicted and would like an outside decision to help you move on (we've been there lol), we vote varX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Looking for feedback and input
Projects
None yet
Development

No branches or pull requests

2 participants