feat(developer): kmc convert typesafety 😎 by SabineSIL · Pull Request #15860 · keymanapp/keyman

SabineSIL · 2026-04-20T15:40:59Z

In several places in kmc-convert: keylayout->kmn we did not care about return type null or undefined for function return values or parameters. Therefore we got several warnings about possible return types not being assignable to symbols.

Even though they did not affect the code running correctly they have been specified more clearly in this PR.

@keymanapp-test-bot skip

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Developer
Keyboards
- Test Keyboards

…vert-Typesafety

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

…vert-Typesafety # Conflicts: # developer/src/kmc-convert/src/keylayout-to-kmn/keylayout-to-kmn-converter.ts # developer/src/kmc-convert/src/keylayout-to-kmn/kmn-file-writer.ts # developer/src/kmc-convert/test/keylayout-to-kmn-converter.tests.ts # developer/src/kmc-convert/test/kmn-file-writer.tests.ts

…vert-Typesafety

ermshiperete

Looks pretty good despite the many comments.

ermshiperete · 2026-05-22T09:23:43Z

+   * @brief  helper function to check if a specific keyMap index exists in a keyMapSet
+   * neccessary because the amount of  <keyMap index> must correspond to
+   * the amount of <keyMapSelect mapIndex>


It would be better to make this full sentences. If this gets processed by a tool the three lines might get put in the same paragraph removing the line breaks, which then would make it hard to understand.

Suggested change

* @brief helper function to check if a specific keyMap index exists in a keyMapSet

* neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>

* @brief Helper function to check if a specific keyMap index exists in a keyMapSet.

* This is neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>.

ermshiperete · 2026-05-22T09:24:16Z

+   * @brief  helper function to check if a specific keyMapSelect index exists in a modifierMap
+   * neccessary because the amount of <keyMap index> must correspond to
+   * the amount of <keyMapSelect mapIndex>


Suggested change

* @brief helper function to check if a specific keyMapSelect index exists in a modifierMap

* neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>

* @brief Helper function to check if a specific keyMapSelect index exists in a modifierMap.

* This is neccessary because the amount of <keyMap index> must correspond to

* the amount of <keyMapSelect mapIndex>.

ermshiperete · 2026-05-22T10:02:13Z

  behavior: string;
  modifier?: string;
-  outchar?: string;
+  outchar?: string | undefined;


This should not be necessary. outchar? makes the property optional which implies undefined. So if you write outchar?: string; the type of outchar is implicitly string|undefined.

ermshiperete · 2026-05-22T10:23:24Z

+        // in case writeCharacterOrUnicode() returns null, the fallback is empty strings for characterMessage.character
+        // and characterMessage.message. Then versionOutputCharacter could be "" and would be written into the kmn file
+        // as ... > '', producing an invalid kmn rule.


I don't understand this comment. It describes what the code does and that this could result in an invalid kmn rule. Wouldn't we want to do something so that we don't get invalid kmn rules? (in which case I'd expect a TODO or something).

Or does the comment try to explain why we have the if ((outputCharacter !== undefined) && (outputCharacter !== "")) check? Then it should mention outputCharacter which could lead to writeCharacterOrUnicode returning false...

(Additional occurrences of the same code comment above)

The explanation would have been:

// in case writeCharacterOrUnicode() returns null, the fallback is empty strings for characterMessage.character // and characterMessage.message. Then versionOutputCharacter will be "" and will be written into the kmn file // as ... > '', preventing an invalid kmn rule like + [NCAPS K_A] > 'undefined'

but this code is removed since writeCharacterOrUnicode(outputCharacter ..) now returns "" in case of null/undefined as input

ermshiperete · 2026-05-22T10:31:37Z

  *         null in case of an empty string or null or undefined input
  */
-  public writeCharacterOrUnicode(ctr: string, msg: string = ""): MessageCharacter {
+  public writeCharacterOrUnicode(ctr: string, msg: string = ""): MessageCharacter | null {


I think it would be better if writeCharacterToUnicode would always returns MessageCharacter. In the error case it could return { character: '', message: '' }. IMO this would simplify things quite a bit.

ermshiperete · 2026-05-22T11:20:59Z

+    ].forEach(function (values) {
+      it(('should convert "' + values[0] + '"').padEnd(25, " ") + 'to "' + values[2] + '"', async function () {
+        const result = sutW.writeCharacterOrUnicode(values[0] as string, values[1] as string);
+        if (result) {


It would be better to split this into two tests: one set that tests the cases where we get a result, and the other for the cases that return null.

ermshiperete · 2026-05-22T11:21:39Z

    });
+    it(('null should create empty string '), async function () {
+        const result1 = sutW.writeDataRules(null);
+        assert.isTrue(result1 === '');


I'd use

Suggested change

assert.isTrue(result1 === '');

assert.equal(result1, '');

done and in other occurrences also

ermshiperete · 2026-05-22T16:28:14Z

    it('run() should throw on unavailable input file name and null output file name', async function () {
      const inputFilename = makePathToFixture('../data/Unavailable.keylayout');
-      const result = sut.run(inputFilename, null);
+      const result = sut.run(inputFilename, undefined);


Since run is async, we'll have to await it:

Suggested change

const result = sut.run(inputFilename, undefined);

const result = await sut.run(inputFilename, undefined);

done here and at other places

ermshiperete · 2026-05-22T16:31:21Z

            this.callbacks.reportMessage(ConverterMessages.Error_UnsupportedCharactersDetected({
              keymapIndex: jsonObj.keyboard.keyMapSet[0].keyMap[i]['index'],
-              output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '',
+              output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '' as string,


as string is unnecessary since it's already a string.

Suggested change

output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '' as string,

output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '',

ermshiperete · 2026-05-22T16:35:16Z

Might be better to initialize this to the empty string, otherwise we could theoretically end up with an uninitialized variable if the if in line 163 evaluates to false.

Suggested change

let versionOutputCharacter = '';

done unused now; use const later

…vert-Typesafety

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

…vert-Typesafety

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

SabineSIL

all comments have been addressed

SabineSIL · 2026-06-03T18:04:06Z

+   * @brief  helper function to check if a specific keyMap index exists in a keyMapSet
+   * neccessary because the amount of  <keyMap index> must correspond to
+   * the amount of <keyMapSelect mapIndex>


SabineSIL · 2026-06-03T18:05:21Z

+   * @brief  helper function to check if a specific keyMapSelect index exists in a modifierMap
+   * neccessary because the amount of <keyMap index> must correspond to
+   * the amount of <keyMapSelect mapIndex>


SabineSIL · 2026-06-03T18:06:20Z

  behavior: string;
  modifier?: string;
-  outchar?: string;
+  outchar?: string | undefined;


SabineSIL · 2026-06-03T18:41:33Z

+        // in case writeCharacterOrUnicode() returns null, the fallback is empty strings for characterMessage.character
+        // and characterMessage.message. Then versionOutputCharacter could be "" and would be written into the kmn file
+        // as ... > '', producing an invalid kmn rule.


The explanation would have been:

// in case writeCharacterOrUnicode() returns null, the fallback is empty strings for characterMessage.character // and characterMessage.message. Then versionOutputCharacter will be "" and will be written into the kmn file // as ... > '', preventing an invalid kmn rule like + [NCAPS K_A] > 'undefined'

but this code is removed since writeCharacterOrUnicode(outputCharacter ..) now returns "" in case of null/undefined as input

SabineSIL · 2026-06-03T18:52:38Z

    const ctr_val = ((m_uni || m_hex || m_dec) ?
-      m_uni ? parseInt(m_uni[1], 16) : m_hex ? parseInt(m_hex[1], 16) : parseInt(m_dec[1], 10) : KeylayoutToKmnConverter.MAX_CTRL_CHARACTER
+      m_uni ? parseInt(m_uni[1], 16) : m_hex ? parseInt(m_hex[1], 16) : m_dec ? parseInt(m_dec[1], 10) : KeylayoutToKmnConverter.MAX_CTRL_CHARACTER : KeylayoutToKmnConverter.MAX_CTRL_CHARACTER


SabineSIL · 2026-06-04T07:10:04Z

+    ].forEach(function (values) {
+      it(('should convert "' + values[0] + '"').padEnd(25, " ") + 'to "' + values[2] + '"', async function () {
+        const result = sutW.writeCharacterOrUnicode(values[0] as string, values[1] as string);
+        if (result) {


SabineSIL · 2026-06-04T07:12:40Z

    });
+    it(('null should create empty string '), async function () {
+        const result1 = sutW.writeDataRules(null);
+        assert.isTrue(result1 === '');


done and in other occurrences also

SabineSIL · 2026-06-04T07:27:05Z

    it('run() should throw on unavailable input file name and null output file name', async function () {
      const inputFilename = makePathToFixture('../data/Unavailable.keylayout');
-      const result = sut.run(inputFilename, null);
+      const result = sut.run(inputFilename, undefined);


done here and at other places

SabineSIL · 2026-06-04T07:30:23Z

done unused now; use const later

SabineSIL · 2026-06-04T07:50:04Z

  *         null in case of an empty string or null or undefined input
  */
-  public writeCharacterOrUnicode(ctr: string, msg: string = ""): MessageCharacter {
+  public writeCharacterOrUnicode(ctr: string, msg: string = ""): MessageCharacter | null {


…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety # Conflicts: # developer/src/kmc-convert/test/data/Test_differentEncodings.keylayout

feat(developer): ensure typesafety for 'return null' and others

19ebc62

SabineSIL added this to the A19S27 milestone Apr 20, 2026

SabineSIL requested review from ermshiperete and mcdurdin April 20, 2026 15:40

SabineSIL self-assigned this Apr 20, 2026

SabineSIL added developer/ mac/ linux/ common/ labels Apr 20, 2026

SabineSIL added this to Keyman Apr 20, 2026

SabineSIL added developer/compilers/ feat common/web/ epic-kmc-convert labels Apr 20, 2026

github-project-automation Bot moved this to Todo in Keyman Apr 20, 2026

keymanapp-test-bot Bot changed the title ~~feat/developer/kmc convert typesafety~~ feat/developer/kmc convert typesafety 😎 Apr 20, 2026

SabineSIL added 4 commits April 20, 2026 17:47

feat(developer): remove tests for entire keyboards

30ea904

feat(developer): read() throw error instead of return null

6802ec4

feat(developer): more typesafety + add tests for validate()

9b9cafe

feat(developer): more typesafety

ef1cf18

SabineSIL changed the title ~~feat/developer/kmc convert typesafety 😎~~ feat(developer):kmc convert typesafety 😎 Apr 21, 2026

SabineSIL changed the title ~~feat(developer):kmc convert typesafety 😎~~ feat(developer): kmc convert typesafety 😎 Apr 21, 2026

SabineSIL and others added 4 commits April 21, 2026 16:44

feat(developer): baseUrl problem of tsconfig.json, errorMsg typo

417c044

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

99892e3

…vert-Typesafety

feat(developer): restore tsconfig.json

4716887

Merge branch 'feat/developer/kmc-convert-Typesafety' of https://githu…

2edf63c

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

SabineSIL marked this pull request as ready for review April 21, 2026 15:44

keyman-server modified the milestones: A19S27, A19S28 Apr 24, 2026

SabineSIL added 3 commits May 4, 2026 20:04

feat(developer): add returntypes

1d3d53e

feat(developer): add tests and comments

9675611

feat(developer): add testfile

302e7b0

keyman-server modified the milestones: A19S28, A19S29 May 11, 2026

SabineSIL moved this from Todo to Code Review in Keyman May 18, 2026

SabineSIL and others added 5 commits May 19, 2026 09:57

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

eb5ac1e

…vert-Typesafety

feat(developer): add missing bracket

5052581

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

b72e98e

…vert-Typesafety

feat(developer): dummy change to start team city

82c8af2

ermshiperete reviewed May 22, 2026

View reviewed changes

keyman-server modified the milestones: A19S29, A19S30 May 23, 2026

SabineSIL and others added 10 commits June 4, 2026 11:11

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

b5e667b

…vert-Typesafety

feat(developer): work on comments of PR review

160f92a

Merge branch 'feat/developer/kmc-convert-Typesafety' of https://githu…

bd351ad

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

Merge branch 'feat/developer/kmc-convert' into feat/developer/kmc-con…

78e4e32

…vert-Typesafety

feat(developer):kmc-convert fix typos in tests

c11e6f8

feat(developer):kmc-convert fix more typos in tests

9dcd90c

feat(developer):kmc-convert fix more typos in tests

0a89802

Merge branch 'feat/developer/kmc-convert-Typesafety' of https://githu…

ed57423

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety

feat(developer): kmc-convert tests for writeCharacterOrUnicode

425ff62

feat(developer): kmc-convert test for warning of html entities

9dedbe1

SabineSIL commented Jun 4, 2026

View reviewed changes

SabineSIL added 3 commits June 4, 2026 20:35

feat(developer): kmc-convert changed test for warning of html entities

974f0c0

Merge branch 'feat/developer/kmc-convert-Typesafety' of https://githu…

d4ac5f3

…b.com/keymanapp/keyman into feat/developer/kmc-convert-Typesafety # Conflicts: # developer/src/kmc-convert/test/data/Test_differentEncodings.keylayout

feat(developer):kmc-convert dummy change to start TeamCity

9361ec9

keyman-server modified the milestones: A19S30, A19S31 Jun 8, 2026

	const result = sut.run(inputFilename, undefined);
	const result = await sut.run(inputFilename, undefined);

	output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '' as string,
	output: jsonObj.keyboard.keyMapSet[0].keyMap[i].key[j]['output'] ?? '',

Uh oh!

Conversation

SabineSIL commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keymanapp-test-bot Bot commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

ermshiperete left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SabineSIL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

SabineSIL commented Apr 20, 2026 •

edited

Loading

keymanapp-test-bot Bot commented Apr 20, 2026 •

edited

Loading

ermshiperete left a comment •

edited

Loading