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

Support :owner/:repo template in repoPaths configuration #343

Conversation

rmacklin
Copy link
Contributor

@rmacklin rmacklin commented Mar 4, 2024

Summary

Previously, we supported a "wildcard" mapping like

user1/*: /path/to/user1_repos/*

to map any repository owned by user1 to the directory of the same name within /path/to/user1_repos/.

But for folks who store all GitHub repositories under a path that includes the owner's name and the repo's name, this would still require them to enter a wildcard entry for every owner. To avoid that, this PR introduces support for an :owner/:repo template in the repoPaths configuration. Now we can specify a repoPaths entry like:

:owner/:repo: /path/to/github.com/:owner/:repo

which will cover everything. It can still be overridden with individual entries that specify either an exact repository or a specific owner wildcard.

How did you test this change?

I added more test cases to ui/common/repopath_test.go. I also built it locally and ran ./gh-dash using an :owner/:repo template in my repoPaths configuration and confirmed that it was now able to successfully check out a PR using my configured location.

Previously, we supported a "wildcard" mapping like
```
user1/*: /path/to/user1_repos/*
```
to map any repository owned by `user1` to the directory of the same name
within `/path/to/user1_repos`.

But for folks who store all GitHub repositories under a path that
includes the owner's name and the repo's name, this would still require
them to enter a wildcard entry for every owner. To avoid that, we're
introducing support for an `:owner/:repo` template in the `repoPaths`
configuration. Now we can specify a `repoPaths` entry like:
```
:owner/:repo: /path/to/github.com/:owner/:repo
```
which will cover everything. It can still be overridden with individual
entries that specify either an exact repository or a specific owner
wildcard.

In case the configured path includes the :`owner` or `:`repo
substitution multiple times, we will replace all occurrences.
Copy link
Owner

@dlvhdr dlvhdr left a comment

Choose a reason for hiding this comment

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

Thank you!

@dlvhdr dlvhdr merged commit 85a75fe into dlvhdr:main Mar 4, 2024
@rmacklin rmacklin deleted the support-owner-repo-template-in-repoPaths-configuration branch March 4, 2024 21:08
@rmacklin
Copy link
Contributor Author

rmacklin commented Mar 4, 2024

Thank you for reviewing and merging! And for creating this cool TUI :)

BTW, one thing that I'm thinking about is that we don't really need the support for default_path anymore - e.g. this configuration:

default_path: ~/code/repos

can be achieved using the more general template support introduced in this PR via:

:owner/:repo: ~/code/repos/:repo

(In contrast, the example in the PR description cannot be achieved using default_path)

Perhaps we could deprecate default_path and remove it in the next major release.

@rmacklin
Copy link
Contributor Author

rmacklin commented Mar 4, 2024

Another option would be to extend default_path with the template functionality, but personally I prefer keeping the :owner/:repo key since it 1) makes the value a bit more self-explanatory and 2) aligns better with the A/B format of the other supported keys:

some-owner/some_repo: /path/to/some_repo
another-owner/*: /path/to/another-owner/*
:owner/:repo: /path/to/:owner/:repo

Additionally, even changing the behavior of default_path to support substitutions could technically be a breaking change (for some users), so if we want to be strict about that we'd probably want to do so in a new major version anyway.

I've opened a PR here for deprecating default_path: #344 - let me know what you think.

@dlvhdr
Copy link
Owner

dlvhdr commented Mar 5, 2024

Yeah totally agreed.
We need to consolidate the way to define these paths and choose one method. I like your suggestion.
:path vars like in urls work well here.
I'm open to introducing a breaking change.

@rmacklin
Copy link
Contributor Author

rmacklin commented Mar 5, 2024

So would you like to go straight to the breaking change and skip the soft deprecation PR? If so, I can close #344 and remove default_path

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.

None yet

2 participants