Skip to content
This repository was archived by the owner on Apr 13, 2020. It is now read-only.

Conversation

@dennisseah
Copy link
Collaborator

Related to microsoft/bedrock#885

  1. move command declaration to a JSON file
  2. standardized on the function in command ts file
  3. use the CommandBuilder's exit function
  4. still default value of command option values is set to "" and values fall back to values in config file, typeof optionValue === "string" is not needed.
  5. added more tests

Copy link
Collaborator

@andrebriggs andrebriggs left a comment

Choose a reason for hiding this comment

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

Thanks @dennisseah. This looks in line with the other refactoring. Does the integration test pass with these changes?

@dennisseah
Copy link
Collaborator Author

Thanks @dennisseah. This looks in line with the other refactoring. Does the integration test pass with these changes?

Yes, I will run it again as i just did a rebase.

@dennisseah
Copy link
Collaborator Author

Thanks @dennisseah. This looks in line with the other refactoring. Does the integration test pass with these changes?

Yes, I will run it again as i just did a rebase.

it is good

Successfully reached the end of the validations scripts.

@andrebriggs andrebriggs self-requested a review February 1, 2020 00:45
Copy link
Collaborator

@andrebriggs andrebriggs left a comment

Choose a reason for hiding this comment

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

Approved pending any integration test re-runs from further commits @dennisseah

@dennisseah
Copy link
Collaborator Author

ran sh validations.sh successful. Merging this PR. Thanks

@dennisseah dennisseah merged commit 7ffaca9 into master Feb 3, 2020
@dennisseah dennisseah deleted the refactorHldPipelineTs branch February 3, 2020 18:08
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