feat(developer): ldml fix all remaining TODOs around markers and variables 🙀 by srl295 · Pull Request #9688 · keymanapp/keyman

srl295 · 2023-10-05T00:23:05Z

finish all of the developer LDML-TODOs around markers and variables

For: #9121
For: #9119

@keymanapp-test-bot skip

- add and re-enable some tests For: #9121 For: #9119

- finish all of the developer LDML-TODOs around markers and variables For: #9121 For: #9119

keymanapp-test-bot · 2023-10-05T00:23:09Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 17.0.187 (0.9688.9370)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

mcdurdin

LGTM but would be good to refactor away the potentially gnarly string replacement snippet

mcdurdin · 2023-10-05T07:54:34Z

common/web/types/src/kmx/kmx-plus.ts

+    if(s === undefined || s === null) {
+      s = '';
+    }


Suggested change

if(s === undefined || s === null) {

s = '';

}

s = s ?? '';

Seems shorter 😁

Ref https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

mcdurdin · 2023-10-05T07:55:47Z

common/web/types/src/kmx/kmx-plus.ts

+    return this.allocList(sections.strs, s.split(' ').map(s => {
+      s = sections.vars.substituteStrings(s, sections);
+      s = sections.vars.substituteMarkerString(s);
+      s = unescapeString(s);
+      return s;
+    }));


Suggested change

return this.allocList(sections.strs, s.split(' ').map(s => {

s = sections.vars.substituteStrings(s, sections);

s = sections.vars.substituteMarkerString(s);

s = unescapeString(s);

return s;

}));

return this.allocList(sections.strs, s.split(' ').map(t => {

t = sections.vars.substituteStrings(t, sections);

t = sections.vars.substituteMarkerString(t);

t = unescapeString(t);

return t;

}));

Reusing s on inner scope is not ideal for readability

mcdurdin · 2023-10-05T07:56:45Z

developer/src/kmc-ldml/src/compiler/keys.ts

+        cookedTo = sections.vars.substituteStrings(cookedTo, sections);
+        cookedTo = sections.vars.substituteMarkerString(cookedTo);
+        const to = sections.strs.allocAndUnescapeString(cookedTo, true);


This pattern is repeated, refactor? It is also similar in disp.ts but written nested for added confusions!

mcdurdin · 2023-10-05T07:57:44Z

developer/src/kmc-ldml/src/compiler/keys.ts

+      cookedLongPressDefault = sections.vars.substituteStrings(cookedLongPressDefault, sections);
+      cookedLongPressDefault = sections.vars.substituteMarkerString(cookedLongPressDefault)
+      const longPressDefault = sections.strs.allocAndUnescapeString(cookedLongPressDefault);


Here's the dup

mcdurdin · 2023-10-05T07:58:01Z

developer/src/kmc-ldml/src/compiler/disp.ts

+      to: sections.strs.allocAndUnescapeString(
+        sections.vars.substituteMarkerString(
+          sections.vars.substituteStrings(display.to, sections))),


This is similar to the code in keys.ts

mcdurdin · 2023-10-05T07:58:10Z

developer/src/kmc-ldml/src/compiler/disp.ts

+      display: sections.strs.allocAndUnescapeString(
+        sections.vars.substituteStrings(display.display, sections)),


This is nearly similar

mcdurdin · 2023-10-05T07:58:29Z

developer/src/kmc-ldml/src/compiler/keys.ts

+      let toCooked = sections.vars.substituteStrings(toRaw, sections);
+      toCooked = sections.vars.substituteMarkerString(toCooked);
      const to = sections.strs.allocAndUnescapeString(toCooked, true);


And dupity dup dup again

srl295 · 2023-10-05T21:29:19Z

@mcdurdin trying to DRY this out a little bit…

      const id = sections.strs.allocString(key.id);
      const longPress: ListItem = sections.list.allocListFromSpaces(
        key.longPress, {
          substituteStrs: true,
          markers: true,
          unescape: true,
        },
        sections);

      const longPressDefault = sections.strs.allocString(key.longPressDefault,
        {
          stringVariables: true,
          markers: true,
          unescape: true,
        },
        sections);

- refactor string preprocessing pipeline into an options bag

srl295 · 2023-10-05T23:00:29Z

Work in progress. tests fail.

mcdurdin · 2023-10-06T03:03:23Z

... {
          substituteStrs: true,
          markers: true,
          unescape: true,
        }, ...

I like. These objects then lend themselves to being constants defined as part of the structural definitions, which is then easy to compare against the spec.

- workaround for debugger/test issue For: #9121 For: #9119

srl295 · 2023-10-06T23:18:27Z

VERY strange

  1) vars
       markers
         should compile: sections/vars/markers-maximal.xml:
     TypeError: source is not iterable
      at new ListItem (file:///Users/srl295/src/keymanapp/keyman/common/web/types/src/kmx/string-list.ts:39:23)
      at ListItem.map (<anonymous>)
      at ListItem.toStringArray (file:///Users/srl295/src/keymanapp/keyman/common/web/types/src/kmx/string-list.ts:88:17)
      at Object.callback (file:///Users/srl295/src/keymanapp/keyman/developer/src/kmc-ldml/test/test-vars.ts:206:54)
      at Context.<anonymous> (file:///Users/srl295/src/keymanapp/keyman/developer/src/kmc-ldml/test/helpers/index.ts:230:18)

basically this /* ListItem */ .map(v => v.toString()) seemed to end up allocating a new ListItem (see stack!), but allocated it with a number such as 2, 3, 4. Not iterable. This code was working before. Anyway ListItem.toString() is only used for tests/debugging, so I just changed it into a for loop which worked fine.

srl295 · 2023-10-07T13:44:05Z

export class ListItem extends Array<ListIndex> implements OrderedStringList {

…

but yet, inside of ListItem.toStringArray():

> this.map(q => q);
Uncaught TypeError TypeError: source is not iterable
    at ListItem (/Users/srl295/src/keymanapp/keyman/common/web/types/src/kmx/string-list.ts:41:23)
    at eval (repl:1:6)

If I'm in a member function, why is it allocating a ListItem? is it looking for a copy constructor?

this works, though

> Array.from(this.values())
(3) [ListIndex, ListIndex, ListIndex]
Array.from(this.values())[0]
á
Array.from(this.values())[1]
é
Array.from(this.values()).map(q => q)
(3) [ListIndex, ListIndex, ListIndex]
Array.from(this.values()).map(q => q.toString())
(3) ['á', 'é', 'í']

so somehow TypeScript's Array does something different when calling map …

- odd, but seems to work this way For: #9121 For: #9119

srl295 · 2023-10-07T13:54:42Z

OK I think i undertstand now.

i've subclassed Array , and changed its constructor
however, Array.map() probably assumes it can call new Array(2) to create an empty 2-element array
… so it ends up calling new ListItem(2) which doesn't make sense (to ListItem)
Perhaps I should instead have a factory static ListItem.from(source: Array<string>, opts: StrsOptions, sections: DependencySections) with the fancy init, and leave the array c'tors alone
This is a classic is-a vs has-a problem, if I HAD an array instead of being one, TS wouldn't get confused.

- Figured it out. Problem was a custom Array constructor. - also updated ElementString class, which would have the same issue. For: #9121 For: #9119

srl295 · 2023-10-07T16:36:35Z

I'll do another check w/ base branch and then merge this.

keyman-server · 2023-10-11T02:51:44Z

Changes in this pull request will be available for download in Keyman version 17.0.187-alpha

srl295 added 2 commits October 4, 2023 18:54

feat(developer): test: more vars and markers 🙀

4114524

- add and re-enable some tests For: #9121 For: #9119

feat(developer): fix vars and markers 🙀

e04b41e

- finish all of the developer LDML-TODOs around markers and variables For: #9121 For: #9119

srl295 requested review from darcywong00, jahorton and mcdurdin as code owners October 5, 2023 00:23

srl295 added this to the A17S23 milestone Oct 5, 2023

srl295 self-assigned this Oct 5, 2023

github-actions bot added developer/ common/ developer/compilers/ common/web/ epic-ldml labels Oct 5, 2023

srl295 changed the title ~~Feat/core/9121-more-vars-epic-ldml~~ feat(developer): ldml fix all remaining TODOs around markers and variables 🙀 Oct 5, 2023

mcdurdin approved these changes Oct 5, 2023

View reviewed changes

feat(common,developer): DRY out strings 🙀

4c0cab2

- refactor string preprocessing pipeline into an options bag

github-actions bot added the feat label Oct 5, 2023

feat(developer): fix vars and markers 🙀

b7c7a1c

- workaround for debugger/test issue For: #9121 For: #9119

Base automatically changed from feat/core/9119-more-markers-epic-ldml to master October 6, 2023 23:27

feat(developer): fix vars and markers 🙀

450e29c

- odd, but seems to work this way For: #9121 For: #9119

feat(developer): fix vars and markers 🙀

12bb554

- Figured it out. Problem was a custom Array constructor. - also updated ElementString class, which would have the same issue. For: #9121 For: #9119

srl295 linked an issue Oct 7, 2023 that may be closed by this pull request

feat(core): ldml marker implementation 🙀 #9119

Closed

4 tasks

Merge branch 'master' into feat/core/9121-more-vars-epic-ldml

505a45e

This was linked to issues Oct 7, 2023

feat(developer): compiler support for marker implementation 🙀 #9117

Closed

feat(core): support ldml transforms 🙀 #7375

Closed

srl295 mentioned this pull request Oct 8, 2023

feat(core): support ldml transforms 🙀 #7375

Closed

4 tasks

srl295 merged commit f5b28aa into master Oct 8, 2023

srl295 deleted the feat/core/9121-more-vars-epic-ldml branch October 8, 2023 00:07

		display: sections.strs.allocAndUnescapeString(
		sections.vars.substituteStrings(display.display, sections)),

Uh oh!

Conversation

srl295 commented Oct 5, 2023

Uh oh!

keymanapp-test-bot bot commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

mcdurdin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

srl295 commented Oct 5, 2023

Uh oh!

srl295 commented Oct 5, 2023

Uh oh!

mcdurdin commented Oct 6, 2023

Uh oh!

srl295 commented Oct 6, 2023

Uh oh!

srl295 commented Oct 7, 2023

Uh oh!

srl295 commented Oct 7, 2023

Uh oh!

srl295 commented Oct 7, 2023

Uh oh!

keyman-server commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

keymanapp-test-bot bot commented Oct 5, 2023 •

edited

Loading