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

feat(developer): kmc analyze osk-char-use 🗜 #8723

Merged
merged 27 commits into from
Jun 12, 2023

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented May 9, 2023

Relates to keymanapp/keyboards#2172.

Adds an analysis module to kmc which initially analyses one or more .kvks or .keyman-touch-layout files, extracting the de-duped and sorted set of key cap strings from those files and printing them, in text, markdown, or json format.

I have created a TODO list in #8969, but none of the items in the list should block merge into kmc-kmw. All TODO items in the source are listed in #8969.

@keymanapp-test-bot skip

@mcdurdin mcdurdin added this to the A17S12 milestone May 9, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented May 9, 2023

@mcdurdin mcdurdin modified the milestones: A17S12, A17S13 May 14, 2023
@mcdurdin mcdurdin force-pushed the feat/typescript-keymanweb-compiler branch from f38fa72 to 6094457 Compare May 14, 2023 05:49
@mcdurdin mcdurdin modified the milestones: A17S13, A17S14 May 28, 2023
Base automatically changed from feat/typescript-keymanweb-compiler to feature-kmc-kmw June 8, 2023 07:38
mcdurdin added 7 commits June 8, 2023 14:42
In order to match existing behaviour with the legacy KeymanWeb compiler,
we should use quoted numbers for row ids. This behaviour can probably be
changed over to the new model in the future, but for now this prevents
unit tests from causing us grief.
mcdurdin added 3 commits June 9, 2023 10:20
Tidies up silent vs quiet vs verbose by adding logLevel as an option.

* Notes that we need to consolidate all the various CompilerOptions
  interfaces before they get too much further out of hand.
* Cleans up a couple of other minor TODO items.
* Also adds info messages for scanning.
@mcdurdin mcdurdin marked this pull request as ready for review June 9, 2023 05:52
@mcdurdin mcdurdin changed the title feat(developer): kmc analyze osk-char-use feat(developer): kmc analyze osk-char-use 🗜 Jun 9, 2023
@mcdurdin mcdurdin modified the milestones: A17S14, A17S15 Jun 10, 2023
@jahorton
Copy link
Contributor

jahorton commented Jun 12, 2023

Preliminary comment: at one point, we had a regression-test system set up for KMW. (Link = its readme.) Of particular note:

Keyman Engine for Web, Keyman Developer (kmcomp, kmcmpdll, kmanalyze) must be built prior to running the tests. (If not testing source versions, only kmanalyze is required).

There is (?) / was (?) an existing kmanalyze tool - will this one mirror that? Noting part of the description above...

Adds an analysis module to kmc

It's clear that this new module isn't (yet) equal to the older kmanalyze. This tool isn't making regression test specs for Web (yet), after all. So... I guess I'm wondering if the near-equal names might cause confusion if they'll be different in the long-term.

Copy link
Contributor

@jahorton jahorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like a lot of log-message stuff got thrown into here without being mentioned... enough so that it would've been nice if it were its own PR. Likely due to the info level being utilized here, making things noisy?

LGTM though.

@mcdurdin
Copy link
Member Author

It's clear that this new module isn't (yet) equal to the older kmanalyze. This tool isn't making regression test specs for Web (yet), after all. So... I guess I'm wondering if the near-equal names might cause confusion if they'll be different in the long-term.

All the existing command line tools are being deprecated. i.e. kmanalyze --> kmc analyze :: kmcomp --> kmc build

So, yes, kmanalyze is deprecated but not yet replaced by kmc analyze. With the libraries in place now for kmc (particularly @keymanapp/common-types), it should be relatively easy to replicate the behaviour of kmanalyze and make a cross-platform replacement; the first one was osk-char-use because that was direct functionality we needed for the OSK font work coming up.

@mcdurdin
Copy link
Member Author

Feels like a lot of log-message stuff got thrown into here without being mentioned... enough so that it would've been nice if it were its own PR. Likely due to the info level being utilized here, making things noisy?

In hindsight, yeah ... probably. The main reason for the log-message tweaks being included here was we needed a 'silent' mode for logging when emitting to console, so that kmc analyze could be used in tees for scripting.

@mcdurdin mcdurdin merged commit a96c38b into feature-kmc-kmw Jun 12, 2023
@mcdurdin mcdurdin deleted the feat/developer/kmc-analyze branch June 12, 2023 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants