-
Notifications
You must be signed in to change notification settings - Fork 25
feat: deprecate the "branch" and "template" flag for the samples command #193
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
- Coverage 63.00% 62.98% -0.03%
==========================================
Files 212 212
Lines 21848 21857 +9
==========================================
Hits 13766 13766
- Misses 7018 7022 +4
- Partials 1064 1069 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
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.
✅ Looks good to me and thanks for adding the (semver:major) comments and deprecation warning!
📝 I left a small suggestion and question. FWIW I'm in favour of deprecated both of these flags to simplify the code and command usage. 👌🏻
| cmd.Flags().StringVarP(&samplesTemplateURLFlag, "template", "t", "", "template URL for your app") | ||
| // DEPRECATED(semver:major): Prefer the create command when repository details are known | ||
| cmd.Flags().StringVarP(&samplesGitBranchFlag, "branch", "b", "", "name of git branch to checkout") | ||
| cmd.Flag("branch").Hidden = 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.
thought: FWIW I think --branch has a legit use-case with the slack samples command. However, I get the idea that slack samples is meant to discover the available templates and isn't meant to target advanced use-cases. The slack create command is designed to handle the general-purpose and advanced ones.
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.
@mwbrooks Thanks for sharing this - I was thinking the same to recommend the create command when creating a specific branch.
I'm wondering if we can revisit this in following change to show samples that have an existing branch when this flag is provided to avoid errors during a git clone?
Will be thinking about this and related docs! 📚
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.
Sounds good! All in all, I'm in favour of a simpler samples command and leaving the advanced usage to create.
Co-authored-by: Michael Brooks <mbrooks@slack-corp.com>
|
@mwbrooks The review and suggestion is appreciated as always! I'm hoping we can move to more opinionated options with these flags and commands with the next |
Changelog
We've deprecated the
--branchand--templateflag for theslack samplescommand. If those were used prior, we recommend using theslack createcommand with these options.Summary
This PR deprecates the the
--branchand--templateflags of theslack samplescommand.Preview
Reviewers
Confirm the
samplescommand continues to work as expected:Requirements