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

Provide more user-friendly generator APIs #7326

Open
1 task done
Tracked by #6828
BeksOmega opened this issue Jul 24, 2023 · 5 comments
Open
1 task done
Tracked by #6828

Provide more user-friendly generator APIs #7326

BeksOmega opened this issue Jul 24, 2023 · 5 comments
Labels
issue: feature request Describes a new feature and why it should be added
Milestone

Comments

@BeksOmega
Copy link
Collaborator

Check for duplicates

  • I have searched for similar issues before opening a new one.

Problem

There are a bunch of methods and properties within the generator code that:

  1. Are protected, when they need to be public so they can be used by block-code generators.
  2. Have confusing names.

The following things are protected but used by generators:

  • quote_ (used by string block)
  • multiline_quote_ (used by multiline string block)
  • nameDB_ (also addressed in Address nameDB_ or add api to get name of a variable #6008)
  • definitions_ (we just cludge imports and user-defined procedures onto this object, you need to know its naming semantics to do this correctly)
  • scrub_ (needs to be called by procedure definition blocks to handle comments)

The provideFunction_ method (which is only supposed to be used for developer-defined functions) is public, but has a sad underscore in the name :/

Request

Provide an API that matches what external developers are actually trying to do.

I think it should look something like:

  • getVariableName
  • getProcedureName
  • quote
  • multilineQuote
  • addProcedure -> This should handle scrubbing and finding a name
  • addImport
  • addDeveloperFunction

Alternatives considered

Make the protected methods public so they're actually usable. But stick with the less-than-ideal API.

Additional context

Related to #6008 (should possibly supercede it)

@BeksOmega BeksOmega added issue: feature request Describes a new feature and why it should be added issue: triage Issues awaiting triage by a Blockly team member labels Jul 24, 2023
@maribethb
Copy link
Contributor

It is probably worth adding some new functions (e.g. we already agreed on getVariableName and getProcedureName, and addImport or addProcedure might also make sense, to be discussed when we get to this change) but let's not rename the functions just to remove underscores. It creates a lot of churn for not a lot of benefit. They should be made public though.

@maribethb maribethb removed the issue: triage Issues awaiting triage by a Blockly team member label Jul 26, 2023
@cpcallen
Copy link
Contributor

let's not rename the functions just to remove underscores. It creates a lot of churn for not a lot of benefit.

If we're changing a bunch of other things in the API then I think we should provide (and document) non-underscored names—but there's no reason to cause churn: the existing underscored names can be kept indefinitely as aliases/wrappers.

@maribethb
Copy link
Contributor

Rachel & I discussed further and still think it's not worth the additional overhead and churn of adding aliases:

  • It would add confusion when developers use blocks that contain a mix of the two styles
  • Either we have dead code around forever, or we deprecate the old ones eventually and cause the unnecessary churn we want to avoid
  • Sets a precedent for what we might do in the rest of Blockly, or else is confusing why we are only adding this special treatment to the CodeGenerator class.
  • Small benefits: In general, the underscores can just be seen as a naming quirk and not a real problem to developers. The main benefit we see is conforming to the style guide for maximal correctness, and that's not a good enough reason to introduce this.

@BeksOmega
Copy link
Collaborator Author

We had someone running into issues with definitions_ being protected on the forum: https://groups.google.com/g/blockly/c/T0IiSWN4R9M

@cpcallen
Copy link
Contributor

The most recent request for this comes from a user who wants to add Python imports. We also do this in generators/python/math.ts (amongst other places) and also workaround via any:

(generator as AnyDuringMigration).definitions_['import_math'] = 'import math';

(If you are working around this issue in your own code use any instead of our internal alias anyDuringMigration.)

@cpcallen cpcallen added this to the Upcoming milestone Jul 19, 2024
@cpcallen cpcallen removed the issue: triage Issues awaiting triage by a Blockly team member label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: feature request Describes a new feature and why it should be added
Projects
None yet
Development

No branches or pull requests

3 participants