Skip to content
This repository has been archived by the owner on Jul 14, 2023. It is now read-only.

Manual config feature gap after refactor #827

Merged

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Apr 18, 2021

No description provided.

@@ -25,8 +25,8 @@ export interface CommandModel {
Command_Mode: string;
Command_Type: string;
Command_GenericSetterArgName: string;
// Command_Features: [string, string];
// Command_Imports: [string, string[]];
Command_Features: Record<string, any>;
Copy link
Member

Choose a reason for hiding this comment

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

the value type of Features is a string? wondering is it necessary to use 'any'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not exactly. I don't want to rule out the possiblity of other complex cases. For example

directive:
    command: some-command-name
feature:
   some_feature: 
       - a
       - 'b'
   some_feature1:
      a1: a2
      'b1': 'b2'

Copy link
Member

Choose a reason for hiding this comment

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

I see. Will it be related to below code? looks currently only string type is handled for "feature"
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this works for both string or number type, I need to add logic about the complex scenario I mentioned above

Copy link
Member

Choose a reason for hiding this comment

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

yes, if it's also supposed to by a number, it will be better to use "prop: string|number"

Copy link
Member Author

Choose a reason for hiding this comment

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

changed,

features:
exception_handler: handle_template_based_exception
imports:
azure.cli.core.exception: handle_template_based_exception
Copy link
Member

Choose a reason for hiding this comment

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

As other pairs are using name-value format, and this seems to be a different pattern, need to verify whether it would cause confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

will discuss with CLI team about it

}
importValues.forEach((val) => {
if (list.indexOf(val) === -1) {
list.push(val);
Copy link
Member

Choose a reason for hiding this comment

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

Does CLI have the requirement to order all the import values?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.

@qiaozha qiaozha merged commit 4780d46 into manual-config-feature-gap Apr 19, 2021
qiaozha added a commit that referenced this pull request Apr 19, 2021
* command-interfaces-user-cases-document

* document update

* group change

* bugfixes and doc improvement

* update group description

* remove new sdk release impact on scenario test

* autorest directive and cli directive document

* refactor of code model impl

* fix codemoel layer

* refactor fixes

* refactor

* reserve work

* refactor the codemodel

* example refactor

* resolve conflict part

* resolve conflicts

* Manual config feature gap after refactor (#827)

* manual-config-feature-gap-after-refactor

* fix-test

* add test config

* manual config features and tests

* scenario test

* add array type example

* feature type

* remove unused reference
@qiaozha qiaozha deleted the manual-config-feature-gap-after-refactor branch April 21, 2021 05:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants