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

[office-addin-dev-settings] Update version of the commander dependency #338

Conversation

TheSamsterZA
Copy link
Contributor

Resolves #337.

@TheSamsterZA TheSamsterZA requested a review from a team as a code owner November 13, 2020 09:09
@ghost
Copy link

ghost commented Nov 13, 2020

CLA assistant check
All CLA requirements met.

@TheSamsterZA
Copy link
Contributor Author

I understand that commander is used across multiple office-addin packages, but was not sure if it was a good idea to update the dependency across all of them in one PR. I decided to limit my change to the package I was having trouble with.

@TCourtneyOwen
Copy link
Contributor

/AzurePipelines run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@TCourtneyOwen
Copy link
Contributor

@TheSamsterZA Please resolve merge conflicts and submit a new iteration.

Thanks,

Courtney

@akrantz
Copy link
Contributor

akrantz commented Nov 13, 2020

Thanks for bringing this to our attention. I think we should update this for all of the packages within office-addin-scripts.

Copy link
Contributor

@akrantz akrantz left a comment

Choose a reason for hiding this comment

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

Let's update this for all of the packages in office-addin-scripts.

@akrantz
Copy link
Contributor

akrantz commented Nov 13, 2020

Also, is there a related update needed for @types/commander?

@akrantz
Copy link
Contributor

akrantz commented Nov 13, 2020

It appears that @types/command is not actually needed. It would be nice to remove that.

@akrantz
Copy link
Contributor

akrantz commented Nov 13, 2020

One other thing, with the major version update to commander, we'll need to ensure that there are no breaking changes which cause a problem. We don't have tests for the CLI itself. The release notes for the major version updates describe breaking changes: https://github.com/tj/commander.js/tags @TCourtneyOwen We can talk offline how best to validate this.

@TheSamsterZA
Copy link
Contributor Author

@akrantz:

Let's update this for all of the packages in office-addin-scripts.

I've submitted a new pull request #339 which does this. Please review.

@TheSamsterZA
Copy link
Contributor Author

Closing since #339 supersedes this PR.

@TheSamsterZA TheSamsterZA deleted the office-add-in-dev-settings/update-commander branch November 14, 2020 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[office-addin-dev-settings] Update commander dependency
3 participants