refactor(generators): Migrate PHP generators to TypeScript #7647

cpcallen · 2023-11-13T19:50:55Z

The basics

I validated my changes

The details

Resolves

Part of #6828.

Proposed Changes

Migrate generators/php/*.js and generators/php.js to TypeScript.

Also make a few minor corrections to previously-migrated files in generators/javascript/ and generators/python/ for consistency (see commit 40ede09).

Reason for Changes

Finish TS migration.

Test Coverage

npm test passes. No changes to manual test procedures required.

Documentation

No changes to documentation yet, as there should be minimal changes to the generated code and no changes to the published typings (because during build we overwrite the generated .d.ts files with the ones from typings/).

Additional Information

It may be easier to review this commit-by-commit, as the final format commit make a lot of noisy changes.

N.B. that individual commit messages contain some additional information about each commit.

Slightly simplify the implementation of getAdjusted, in part to make it more readable. Also improve its JSDoc comment.

First pass doing very mechanistic migration, not attempting to fix all the resulting type errors.

This consists almost entirely of adding casts, so the code output by tsc should be as similar as possible to the pre-migration .js source files.

The migration of the JavaScript and Python generators inadvertently introduced some inconsistencies in the code, e.g. putting executable code before the initial comment line that most generator functions begin with. This fixes another instance of this (but n.b. that these inline comments should have been JSDocs and a task has been added to google#7600 to convert them). Additionally, I noticed while doing the PHP migration that ORDER_OVERRIDES was not typed as specifically as it could be, in previous migrations, so this is fixed here (along with the formatting of the associated JSDoc, which can fit on one line now.)

The way the generator functions are added to phpGenerator.forBlock has been modified so that incorrect generator function signatures will cause tsc to generate a type error.

One block protected with // prettier-ignore to preserve careful comment formatting. Where there are repeated concatenations prettier has made a pretty mess of things, but the correct fix is probably to use template literals instead (rather than just locally disabling prettier). This is one of the items in the to-do list in google#7600.

With an update to the wording for generators/php.ts.

maribethb

Overall lgtm, minor questions and comments

generators/javascript/lists.ts

generators/php/lists.ts

maribethb · 2023-11-21T01:53:45Z

generators/php/loops.ts

@@ -29,11 +32,11 @@ export function controls_repeat_ext(block, generator) {
 branch = generator.addLoopTrap(branch, block);
 let code = '';
 const loopVar =
- generator.nameDB_.getDistinctName('count', NameType.VARIABLE);
+ generator.nameDB_!.getDistinctName('count', NameType.VARIABLE);


I think we could improve this code by checking that nameDB_ exists and if not tell people they need to initialize the generator. This is a common mistake and it's actually not safe to assert this exists imo.

Or we could add a getDistinctName to the Generator class, similar to this one, which would do the check for us so we don't repeat it in every place and don't need to assert on the nameDB

I think we could improve this code by checking that nameDB_ exists and if not tell people they need to initialize the generator. This is a common mistake and it's actually not safe to assert this exists imo.

Agreed. This is the first item in the to-do list in #7600—but I added a note specifically mentioning a check.

Or we could add a getDistinctName to the Generator class, similar to this one, which would do the check for us so we don't repeat it in every place and don't need to assert on the nameDB

That's an even better idea, which I also added to #7600.

generators/php/math.ts

generators/php/php_generator.ts

maribethb · 2023-11-21T02:13:59Z

generators/php/php_generator.ts

+ if (stringUtils.isNumber(at)) {
+ at = String(Number(at) + delta);
+ if (negate) {
+ at = String(-Number(at));


I guess at has to be a string for TS since valueToCode will return a string. but the converting back and forth between string and number is really ugly. would it look better if you used a different variable and only convert it to string when you're returning that value?

A good suggestion. I think this is best dealt with separately, however, so that all the implementations can be improved at the same time and perhaps made more consistent in the process, so I've again added it to the list in #7600.

maribethb · 2023-11-21T02:18:39Z

generators/php/text.ts

 // Text value.
 const code = generator.multiline_quote_(block.getFieldValue('TEXT'));
 const order =
 code.indexOf('.') !== -1 ? Order.STRING_CONCAT : Order.ATOMIC;
 return [code, order];
 };

-export function text_join(block, generator) {
+export function text_join(block: Block, generator: PhpGenerator): [string, Order] {


would it work to just type block as JoinMutatorBlock to begin with? haven't thought through the implications of this, just throwing it out there.

That would be a great idea but alas it is not possible to put a (block: JoinMutatorBlock, …) => … into the phpGenerator.forBlock dictionary because that type is not assignable to (block: Block, …) => … (which is what the forBlock dictionary is declared as containing).

I've been thinking about this issue for a while, and I think the correct approach is to for the block definitions to provide type predicates (for at least all those block type that declare additional properties) and then have the generator functions use those type predicates to narrow the type of block via runtime test (e.g. if (!isJoinMutatorBlock(block)) throw new TypeError(), after which block is understood to be a JoinMutatorBlock).

That obviously entails providing such predicates, and possibly changing the way the blocks types are declared, so I think this is out-of-scope for this PR.

- Don't declare unused wherePascalCase dictionary. - Don't allow null in OPERATOR dictionary when not needed. - Fix return type (and documentation thereof) of getAdjusted.

@Protected

In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ was not transcribed to the new typescript signature. BREAKING CHANGE: This makes scrub_ protected again, which will break any code attempting to access this method from outside a CodeGenerator subclass.

@Protected

Fixes google#2156. In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found google#2156. Per discussion amongst team, I am not renaming it to scrub at this time.

@Protected

Fixes google#2156. In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found google#2156. Per discussion amongst team, I am not renaming it to scrub at this time.

@Protected

Fixes google#2156. In PRs google#7602, google#7616, google#7646, google#7647 and google#7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found google#2156. Per discussion amongst team, I am not renaming it to scrub at this time.

@Protected

Fixes #2156. In PRs #7602, #7616, #7646, #7647 and #7654 the @Protected access modifier on scrub_ on the CodeGenerator subclasses was not transcribed to the new typescript signature. I was going to re-add it, but this breaks some of the procedure block generator functions which rely on it, and then @BeksOmega pointed out that this might be one of the CodeGenerator API functions which we had already decided should be public—and lo and behold I found #2156. Per discussion amongst team, I am not renaming it to scrub at this time.

cpcallen added 7 commits November 13, 2023 13:26

refactor(generators): Migrate dart_generator.js to TypeScript

9fec824

refactor(generators): Simplify getAdjusted

bae0d63

Slightly simplify the implementation of getAdjusted, in part to make it more readable. Also improve its JSDoc comment.

refactor(generators): Migrate generators/php/* to TypeScript

86a2e72

First pass doing very mechanistic migration, not attempting to fix all the resulting type errors.

fix(generators): Fix type errors in generator functions

e2100eb

This consists almost entirely of adding casts, so the code output by tsc should be as similar as possible to the pre-migration .js source files.

refactor(generators): Migrate generators/php.js to TypeScript

5d0ac54

The way the generator functions are added to phpGenerator.forBlock has been modified so that incorrect generator function signatures will cause tsc to generate a type error.

cpcallen added component: generators PR: refactor labels Nov 13, 2023

cpcallen requested a review from maribethb November 13, 2023 19:50

cpcallen requested a review from a team as a code owner November 13, 2023 19:50

github-actions bot assigned maribethb Nov 13, 2023

github-actions bot added PR: refactor and removed PR: refactor labels Nov 13, 2023

docs(generators): @fileoverview -> @file

8686f5d

With an update to the wording for generators/php.ts.

cpcallen force-pushed the refactor/6828/php branch from 2ae9ccf to 8686f5d Compare November 18, 2023 00:23

maribethb approved these changes Nov 21, 2023

View reviewed changes

cpcallen mentioned this pull request Nov 21, 2023

Improve implementation and type safety of generators #7600

Open

10 tasks

fix(generators): Fixes for PR google#7647.

4e87ae5

- Don't declare unused wherePascalCase dictionary. - Don't allow null in OPERATOR dictionary when not needed. - Fix return type (and documentation thereof) of getAdjusted.

maribethb approved these changes Nov 21, 2023

View reviewed changes

cpcallen mentioned this pull request Nov 21, 2023

fix(generators): Improve consistency of migrated language generators #7662

Merged

1 task

cpcallen merged commit 7bda309 into google:develop Nov 28, 2023
6 checks passed

cpcallen deleted the refactor/6828/php branch November 28, 2023 16:59

cpcallen mentioned this pull request Mar 15, 2024

fix(generators): Make scrub_ public #7940

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(generators): Migrate PHP generators to TypeScript #7647

refactor(generators): Migrate PHP generators to TypeScript #7647

cpcallen commented Nov 13, 2023

maribethb left a comment

maribethb Nov 21, 2023

cpcallen Nov 21, 2023

maribethb Nov 21, 2023

cpcallen Nov 21, 2023

maribethb Nov 21, 2023

cpcallen Nov 21, 2023

refactor(generators): Migrate PHP generators to TypeScript #7647

refactor(generators): Migrate PHP generators to TypeScript #7647

Conversation

cpcallen commented Nov 13, 2023

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

maribethb left a comment

Choose a reason for hiding this comment

maribethb Nov 21, 2023

Choose a reason for hiding this comment

cpcallen Nov 21, 2023

Choose a reason for hiding this comment

maribethb Nov 21, 2023

Choose a reason for hiding this comment

cpcallen Nov 21, 2023

Choose a reason for hiding this comment

maribethb Nov 21, 2023

Choose a reason for hiding this comment

cpcallen Nov 21, 2023

Choose a reason for hiding this comment