-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
fix(developer): make kmc log options consistent across all commands #13075
fix(developer): make kmc log options consistent across all commands #13075
Conversation
Makes the `--log-format`, `--log-level`, and `--color`/`--no-color` options available for all commands and DRYs out some of the options processing for consistency. This has positive impact in particular on the Generator classes which removes the need for several unit tests as the interface can now be checked at compile time. Fixes: #13072 Unblocks: #13073
User Test ResultsTest specification and instructions
Test Artifacts
|
@@ -45,32 +46,11 @@ describe('generateKeymanKeyboard', function() { | |||
outPath = null; | |||
}); | |||
|
|||
it('should only allow a single id', async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two tests are no longer necessary as they are validated at compile-time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
const keyboardOptions = { | ||
icon: true, | ||
const keyboardOptions: GeneratorOptions = { | ||
// icon: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this supposed to be part of #12971?
Same change on the other tests.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was missed because it was essentially ignored (because it was an untyped object). Once I added the GeneratorOptions
type, that forced the removal of the icon
property. No real harm done, fortunately!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice DRY work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Also add some resilience to option usage in kmc-generate; more work may need to be done here in future. Fixes: #13110
Test ResultsI tested this issue with the attached "Keyman-18.0.178-alpha-test-13075" build(03/02/2025) on Windows 10. I'm sharing my observation here. GROUP_ANALYZE_OSK_CHAR_USE:
|
Hi @dinakaranr, could you let me know what the failures were here please? |
Thank you @dinakaranr. I think the generate command does not currently generate any logs unless there is an error. We should add some 'info' logs for normal operation, which I will put into a new issue to resolve during beta. So I think we can say that these passed, hurrah! GROUP_GENERATE_KEYMAN_KEYBOARD: kmc generate keyman-keyboard |
Changes in this pull request will be available for download in Keyman version 18.0.185-alpha |
Makes the
--log-format
,--log-level
, and--color
/--no-color
options available for all commands and DRYs out some of the options processing for consistency. This has positive impact in particular on the Generator classes which removes the need for several unit tests as the interface can now be checked at compile time.Fixes: #13072
Fixes: #13110
Unblocks: #13073
User Testing
We need to test that the logging options work consistently for all commands in kmc. Run tests for each of the commands below:
GROUP_ANALYZE_OSK_CHAR_USE:
kmc analyze osk-char-use
GROUP_ANALYZE_REWRITE_FROM_OSK_CHAR_USE:
kmc analyze osk-rewrite-from-char-use
GROUP_BUILD_FILE:
kmc build file
GROUP_BUILD_LDML_TEST_DATA:
kmc build ldml-test-data
GROUP_BUILD_WINDOWS_PACKAGE_INSTALLER:
kmc build windows-package-installer
GROUP_MESSAGE:
kmc message
GROUP_GENERATE_KEYMAN_KEYBOARD:
kmc generate keyman-keyboard
GROUP_GENERATE_LDML_KEYBOARD:
kmc generate ldml-keyboard
GROUP_GENERATE_LEXICAL_MODEL:
kmc generate lexical-model
GROUP_COPY:
kmc copy
TEST_LOG_LEVEL_SILENT: run the command with
--log-level silent
and verify that all messages are hiddenTEST_LOG_FORMAT_TSV: run the command with
--log-format tsv
and verify that all messages are printed in a tabular formatTEST_COLOR: run the command with
--no-color --log-level verbose
and verify that all printed messages have no color applied