Don't create duplicated functions for code repositories and wiki repositories #33924
Conversation
lunny
added
the
type/refactoring
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/L
github-actions
bot
added
modifies/api
GiteaBot
added
lgtm/need 1
| return string(sr) | ||
| } | ||
|
|
||
| func (repo *Repository) WikiStorageRepo() StorageRepo { |
There was a problem hiding this comment.
It's a storage repository interface, not only path.
| return RelativePath(repo.OwnerName, repo.Name) | ||
| } | ||
|
|
||
| type StorageRepo string |
There was a problem hiding this comment.
may RepoPath can cause confusion with repo url path? maybe StoragePath?
There was a problem hiding this comment.
There is already a complex Storage system in Gitea.
TBH any new variable named "StorageXxx" doesn't look good to me (but I won't say block)
There was a problem hiding this comment.
may RepoPath can cause confusion with repo url path?
No, I would say it does not:
After all, we are on the models layer here.
As such, it is far more likely that we are talking about how the repo is stored compared to how it is accessed.
If it had something to do with URLs, we would need the URL somewhere in the name, since that stuff is normally handled by routers and services.
There was a problem hiding this comment.
It maybe refactored to add more interface functions rather than repo path, so I don't think it's a good name as RepoPath. And this is an implementation of gitrepo.Repository.
| } | ||
|
|
||
| func (repo *Repository) WikiStorageRepo() StorageRepo { | ||
| return StorageRepo(strings.ToLower(repo.OwnerName) + "/" + strings.ToLower(repo.Name) + ".wiki.git") |
There was a problem hiding this comment.
We could check here that we don't already have a Wiki repo:
It could be possible in the future that someone calls this method unaware that it's already a wiki repo, and receives a double-encoded wiki repo (.wiki.git.wiki.git).
There was a problem hiding this comment.
I want to add a comment here: currently, Repository refers to the entire concept of a repository. I don’t believe it can be interpreted to mean a wiki repository specifically.
GiteaBot
added
lgtm/done
|
While this PR has two approvals already, I'd certainly like to wait for @wxiaoguang's review before merging as he proposed such a change in the first place. |
Thank you very much for asking review. Sometimes I block PRs if something would go wrong. For this one, I didn't see obvious problem (no block) but I don't know whether it is the right way (no approval). So feel free to approve and merge if you have reviewed. |
|
The name could be changed if there is a better one. Since this abstract will make it easier and less change, I think we can move forward at the moment. |
lunny
added
the
reviewed/wait-merge
GiteaBot
removed
the
reviewed/wait-merge
Fix #33910 (review)
This PR changed the Repositroy interface in
gitrepopackage which makes it only focus the relative path in the disk and abstract whether it's a wiki repository or not.