Skip to content

Improve documentation and consistency of the --commit flag for gh browse command#7105

Merged
samcoe merged 2 commits intocli:trunkfrom
alex-petrov-vt:iss-6926
Mar 13, 2023
Merged

Improve documentation and consistency of the --commit flag for gh browse command#7105
samcoe merged 2 commits intocli:trunkfrom
alex-petrov-vt:iss-6926

Conversation

@alex-petrov-vt
Copy link
Contributor

@alex-petrov-vt alex-petrov-vt commented Mar 8, 2023

This is a follow up PR to the discussion in #6926

Before, the --commit flag for the gh browse command always generated the /tree repository url for the latest commit.

This PR adjusts the --commit flag to match the --branch flag behavior. The --commit flag now accepts an arbitrary commit sha and generates the /tree url for the repository at that commit. It also defaults to the latest commit, if no argument is provided to the flag, to maintain backwards compatibility.

Fixes #6926 (maybe 😆)

Before, the --commit flag for the `gh browse` command always generated
the `/tree` repository url for the latest commit.

This commit adjusts the `--commit` flag to match the `--branch` flag
behavior. The `--commit` flag now accepts an arbitrary commit sha and
generates the `/tree` url for the repository at that commit. It also
defaults to the latest commit, if no argument is provided to the flag,
to maintain backwards compatibility.

// To preserve backwards compatibility when commit flag used to be a
// boolean flag.
cmd.Flags().Lookup("commit").NoOptDefVal = emptyCommitFlag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't think of a better way of maintaining backwards compatibility while switching the commit flag from a boolean flag to a string flag.

This seemed like a suggested way of doing it by the pflag: https://pkg.go.dev/github.com/spf13/pflag#readme-setting-no-option-default-values-for-flags

However, this means that the commit flag now must use an equals sign to assign the flag value (i.e. --commit=12a4) which looks pretty inconsistent compared to the --branch flag, for example, which just expects a typical string argument after itself.

Please let me know if you think there's a way to improve this

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is unfortunately the only way to do this. I think it is good for now and then in a future release, perhaps when we do another major release, we can remove the NoOptDefVal and break the backwards compatibility.

@alex-petrov-vt alex-petrov-vt marked this pull request as ready for review March 11, 2023 18:57
@alex-petrov-vt alex-petrov-vt requested a review from a team as a code owner March 11, 2023 18:57
@alex-petrov-vt alex-petrov-vt requested review from samcoe and removed request for a team March 11, 2023 18:57
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Mar 11, 2023
@samcoe samcoe self-assigned this Mar 11, 2023
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

@alex-petrov-vt Thanks for this! The code looks good to me. I pushed a small commit to cleanup some of the already present logic in the command to make it easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gh browse -cn should return the commit view not tree view

3 participants