-
Notifications
You must be signed in to change notification settings - Fork 157
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
[one-cmds] Revise one-codegen #14120
base: master
Are you sure you want to change the base?
Conversation
@@ -5,7 +5,6 @@ one-codegen=True | |||
target=onecc_066 | |||
|
|||
[one-codegen] | |||
invalid_option=onecc_066 |
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.
From now, non-registered options in command schema raises an error.
This commit revises one-codegen. - Revise the overall logic to make it easy to maintain. - Rename confusing variables. - Add missing checks for input validation. ONE-DCO-1.0-Signed-off-by: seongwoo <[email protected]>
if grep -q "error: 'command' key is missing in the configuration file" "${filename}.log"; then | ||
if grep -q "error: Not found the command for given backend" "${filename}.log"; then |
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.
Before and after the commit of this test catches the same negative case.
[1] one-codegen -h | ||
[2] one-codegen -v | ||
[3] one-codegen -C ${cfg} (backend, command key in cfg) | ||
[4] one-codegen -C ${cfg} (backends key in cfg) | ||
[5] one-codegen -b ${backend} ${command} | ||
[6] one-codegen -b ${backend} -- ${command} | ||
[7] one-codegen -b ${backend} -C {cfg} (backend, command key in cfg) | ||
[8] one-codegen -b ${backend} -C {cfg} (backends key in cfg) (Only 'backend' is invoked, | ||
even though cfg file has multiple backends) | ||
[9] one-codegen -b ${backend} -C ${cfg} -- ${command} (backend, command key in cfg) | ||
[10] one-codegen -b ${backend} -C ${cfg} -- ${command} (backends key in cfg) (Only 'backend' is invoked, | ||
even though cfg file has multiple backends) | ||
[11] one-codegen -C {cfg} (w/ target, w/ command schema) | ||
[12] one-codegen -C {cfg} (w/ target, w/o command schema) | ||
[13] one-codegen -C {cfg} -T {target} (w/ command schema) | ||
[14] one-codegen -C {cfg} -T {target} (w/o command schema) | ||
[15] one-codegen -T {target} ${command} (ignore command schema) | ||
[16] one-codegen -T {target} -- ${command} (ignore command schema) |
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.
This comments has been just pulled up from below L175.
@mhs4670go , change is a bit huge to concentrate and review. |
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
I'm also worried about whether the change doesn't have any bugs.. At least, this change passed all the current tests. So, I can say it is okay. |
@lemmaa PTAL |
1 similar comment
@lemmaa PTAL |
Sorry. I'll look into it as soon as possible. |
This commit revises one-codegen.
Draft: #14005
ONE-DCO-1.0-Signed-off-by: seongwoo [email protected]