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

Allow configuring prefix for referencing Issue and PRs #9088

Closed
davidsvantesson opened this issue Nov 20, 2019 · 24 comments · Fixed by #9116
Closed

Allow configuring prefix for referencing Issue and PRs #9088

davidsvantesson opened this issue Nov 20, 2019 · 24 comments · Fixed by #9116
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@davidsvantesson
Copy link
Contributor

  • Gitea version (or commit ref): 1.10.0

Description

Currently hash (#) is used to reference both PRs and issues. As issues and PRs uses the same number series per repository in Gitea this is not a problem. But if you use external issue tracker all references will refer to the external site, and it is not possible to refer a PR (except with the full url).

My suggestion is to allow configuring the prefix for references issues and PRs (independently) so it is possible to distinguish them. So in addition to the default (#) you can use e.g.

  • another symbol ($, ¤, !).
  • several letters like #PR123, #I123

Maybe anything can be allowed, but there should be some recommendation on suitable prefixes to not clash with other uses (e.g. '@' would be unsuitable).
The configuration could be made in app.ini (for site default) and per repo in settings.

Do you think this would be doable? Any comments?

@guillep2k
Copy link
Member

I think it's doable, but I wouldn't make it freely configurable. I'd chose one alternate symbol set and let the user chose between them either for issues or PRs. ¤ is not available in all keyboards, but % should be.

@guillep2k
Copy link
Member

Note: I'd have a three level setting in app.ini: no change, only for repositories with external trackers, or for all repositories. The no change setting is to maintain compatibility with existing repositories. I'd use the app.ini value as default for new reposotiries, and then the actual value should be stored in the repo itself. An UI to change it can be added in a later PR.

@davidsvantesson
Copy link
Contributor Author

I think the hash sign is quite general accepted as reference for issues. The only thing I want to add is the ability to specify if reference to an issue or pr. So I am not sure there really is a need to change what symbol to use for referencing, or if a setting is needed...

Something I was positively surprised of was that Gitea already supports referring to issues in other organizations and repositories (in the format go-gitea/gitea#9088). Can we extend that to support specifying if the reference is to pr or issue? E.g. PR#9088 and ISSUE#9088 (preferably case insensitive so pr#9088 works as well). If you don't specify either it defaults to issue as today (redirected to pull if internal issue tracker is used). Possibly the default (pr or issue) can be a setting.

In combination with specifying repo it would be go-gitea/gitea/pr#9088 and go-gitea/gitea/issue#9088 (or is the reversed order better?)

@techknowlogick techknowlogick added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Nov 20, 2019
@guillep2k
Copy link
Member

I think the hash sign is quite general accepted as reference for issues. The only thing I want to add is the ability to specify if reference to an issue or pr. So I am not sure there really is a need to change what symbol to use for referencing, or if a setting is needed...

Something I was positively surprised of was that Gitea already supports referring to issues in other organizations and repositories (in the format go-gitea/gitea#9088). Can we extend that to support specifying if the reference is to pr or issue? E.g. PR#9088 and ISSUE#9088 (preferably case insensitive so pr#9088 works as well). If you don't specify either it defaults to issue as today (redirected to pull if internal issue tracker is used). Possibly the default (pr or issue) can be a setting.

In combination with specifying repo it would be go-gitea/gitea/pr#9088 and go-gitea/gitea/issue#9088 (or is the reversed order better?)

You're right, currently there's no way of referencing a PR in the same repo. But you've also made me just realize that the cross-repo references are completely broken when they are in or refer to a repo with external issue tracker because the repo.metas() are always taken from the repository of the rendered content. 😢

Anyway, we can allow PR# for repos with external issue trackers, but that will be a problem for cross references too. Oh, it's very complicated!

@guillep2k
Copy link
Member

guillep2k commented Nov 20, 2019

I'm trying to complete this table, can you help me?

Reference in User1/Repo1 Repo1 issues are external RepoZ issues are external Should render
#1234 no N/A A link to issue/pull 1234 in User1/Repo1
User1/Repo1#1234 no N/A A link to issue/pull 1234 in User1/Repo1
UserZ/RepoZ#1234 N/A no A link to issue/pull 1234 in UserZ/RepoZ
#1234 yes N/A A link to external issue 1234 for User1/Repo1
User1/Repo1#1234 yes N/A A link to external issue 1234 for User1/Repo1
UserZ/RepoZ#1234 N/A no A link to issue/pull 1234 in UserZ/RepoZ
UserZ/RepoZ#1234 N/A yes A link to external issue 1234 for UserZ/RepoZ
PR#1234 N/A N/A A link to PR 1234 in User1/Repo1
User1/Repo1 ??? N/A N/A A link to PR 1234 in User1/Repo1
UserZ/RepoZ ??? N/A N/A A link to PR 1234 in UserZ/RepoZ
- - - -
AAA-1234 yes N/A A link to external issue AAA-1234 for User1/Repo1
User1/Repo1 ??? AAA-1234 yes N/A A link to external issue AAA-1234 for User1/Repo1
UserZ/RepoZ ??? AAA-1234 N/A yes A link to external issue AAA-1234 for UserZ/RepoZ

The latest section if for issues with alphanumeric format (I guess some external tracker uses that).

@davidsvantesson
Copy link
Contributor Author

I just looked in a bit on alphanumeric a bit and I don't think they work as you specified, they seem to be without hash (see regex).

You have typo on row 6 (wrong repo)

@guillep2k
Copy link
Member

I just looked in a bit on alphanumeric a bit and I don't think they work as you specified, they seem to be without hash (see regex).

You're right. I forgot. I wrote that myself. 😋

@davidsvantesson
Copy link
Contributor Author

davidsvantesson commented Nov 20, 2019

I think we should have

  • User1/Repo1/PR#1234
  • UserZ/RepoZ/PR#1234

Also I think it would be neat to support this:

  • RepoX#1234
  • RepoX/PR#1234
    (org default to the same as repo that reference is made from).

But maybe that is problematic if someone fork the repo.... ?

@guillep2k
Copy link
Member

But maybe that is problematic if someone fork the repo.... ?

I don't think forks are problematic as they don't migrate issues/PRs.

RepoX/#1234 and User1/Repo1/PR#1234 do not conform to the currently supported syntax (which we must continue to support). Somehow seems a little hacky? User1/Repo1#PR1234 sounds like a better fit to me, but I just saw another user use the PR#1234 syntax in a PR on this very repo, which makes me think that other systems use it (GitHub ignored it, by the way).

The thing is that too many options will become a burden, because it limits what you can write without creating a reference (you cannot choose if the reference is created or not). References will be seen in the referenced issue/PR as a comment and can't be deleted (they will be formatted with strike through if removed).

@davidsvantesson
Copy link
Contributor Author

davidsvantesson commented Nov 20, 2019

I don't understand why it doesn't conform to current syntax, it would still support both User1/Repo1#1234 and User1/Repo1/PR#1234. So I don't see a problem with backwards compatibility. This change shouldn't force specifying if the link is to issue or pr, the default would still be issues (possibly configurable).

Regarding RepoX/#1234 it would be the same. Only if someone have written RepoX/#1234 somewhere today and don't expect it to render a reference it would break the backwards compatibility. The problem with forks can be that after forking the repo is not in OrgX any longer but under UserY, so the cross reference would change to try to find the same repo under UserY (which doesn't have the issue).

At least I think the style should be the same for internal and cross-references, so either

  • PR#1234 and org/repo/PR#1234, OR
  • #PR1234 and org/repo#PR1234

@guillep2k
Copy link
Member

I don't understand why it doesn't conform to current syntax, it would still support both User1/Repo1/#1234 and User1/Repo1/PR#1234.

You've got an extra /. It's: User1/Repo1#1234, not User1/Repo1/#1234.

@davidsvantesson
Copy link
Contributor Author

Sorry, that shouldn't be there. Only if you want to specify it's an PR reference you need the extra slash, so User1/Repo1#1234 is the normal syntax and User1/Repo1/PR#1234 to specify it's a PR reference.

@guillep2k
Copy link
Member

Sorry, that shouldn't be there. Only if you want to specify it's an PR reference you need the extra slash, so User1/Repo1#1234 is the normal syntax and User1/Repo1/PR#1234 to specify it's a PR reference.

That's what I find hacky. But perhaps it's the best we can do. How do you feel about #PR1234? It shouldn't clash with anything.

@davidsvantesson
Copy link
Contributor Author

I prefer having it before, partly because #PR1234 looks like another issue numbering system (quite similar to the alphanumeric). But I think it is mostly a question of taste.

Maybe the alphanumeric style should be left out from this, at last for now? It would limit that you can't refer to an alphanumeric issue on another repo, but you can still refer to both PRs and numeric issues. I'm afraid it would add to many clashes with normal written text.

@davidsvantesson
Copy link
Contributor Author

Looking at some other systems..

My opinion would be to either go for GitLab way (!1234 for referencing PR), or similar to BitBucket but maybe with only PR#1234 instead of pull request #1234.

Would people expect this to be localized, so instead of PR you can use something else? Exclamation mark has the benefit of not need to be localized.

@guillep2k
Copy link
Member

Maybe the alphanumeric style should be left out from this, at last for now?

Currently it doesn't support cross-references, so it's OK for me. The problem is that once we decide for one format we won't be able to change it in the future, so we must be aware of the choices we make.

I prefer having it before, partly because #PR1234 looks like another issue numbering system (quite similar to the alphanumeric). But I think it is mostly a question of taste.

I'm not against PR#1234 when it's alone, but I don't like adding a slash for user1/repo1/PR#1234 because we currently don't use it, and I'd go for consistency rather than aesthetics. My view on the options:

  • PR#1234 - pros: looks natural, users won't need to "think" to apply it for local PRs; cons: clashes with the current system, it's not i18n.
  • #PR1234 - pros: kinda looks natural (PR can be regarded as a clarification), doesn't clash with the current system; cons maybe it looks like a new numbering scheme?, it's not i18n.
  • %1234 - pros i18n friendly, doesn't clash with the current system; cons difficult to remember (what symbol was it, again?)
  • #1234PR - pros: kinda looks natural (PR can be regarded as a clarification) but not so much, doesn't clash with the current system; cons maybe it looks like a new numbering scheme?, it's not i18n.

@davidsvantesson
Copy link
Contributor Author

davidsvantesson commented Nov 20, 2019

If we use a symbol I would suggest using exclamation mark as GitLab, no need to reinvent the wheel and easier for people switching to Gitea.

I think we can also consider the Bitbucket way:

  • pull request #1234. pros: natural text + it supports cross-reference fully compatible with current system (pull request org/repo#1234); cons: it's not i18n.

@guillep2k
Copy link
Member

I didn't know about GitLab. The Bitbucket way seems very difficult to implement correctly. So, this?:

Reference in User1/Repo1 Repo1 issues are external RepoZ issues are external Should render
#1234 no N/A A link to issue/pull 1234 in User1/Repo1
!1234 no N/A A link to issue/pull 1234 in User1/Repo1
#1234 yes N/A A link to external issue 1234 for User1/Repo1
!1234 yes N/A A link to PR 1234 for User1/Repo1
User1/Repo1#1234 no N/A A link to issue/pull 1234 in User1/Repo1
User1/Repo1!1234 no N/A A link to issue/pull 1234 in User1/Repo1
User1/Repo1#1234 yes N/A A link to external issue 1234 for User1/Repo1
User1/Repo1!1234 yes N/A A link to PR 1234 for User1/Repo1
UserZ/RepoZ#1234 N/A no A link to issue/pull 1234 in UserZ/RepoZ
UserZ/RepoZ!1234 N/A no A link to issue/pull 1234 in UserZ/RepoZ
UserZ/RepoZ#1234 N/A yes A link to external issue 1234 for UserZ/RepoZ
UserZ/RepoZ!1234 N/A yes A link to PR 1234 for UserZ/RepoZ
Alphanumeric issue IDs: - - -
AAA-1234 yes N/A A link to external issue AAA-1234 for User1/Repo1
!1234 yes N/A A link to PR 1234 for User1/Repo1
User1/Repo1!1234 yes N/A A link to PR 1234 for User1/Repo1
N/A N/A yes A link to external issue AAA-1234 for UserZ/RepoZ
UserZ/RepoZ!1234 N/A yes A link to PR 1234 in UserZ/RepoZ

The last section is for repositories with external trackers that use alphanumeric format.

@guillep2k
Copy link
Member

So, I'm working on this based in the table above. Let's see how other people like it. 😄

@davidsvantesson
Copy link
Contributor Author

With the localization in mind, I think the exclamation mark is the way to go.

The feature will be slightly breaking. However I don't think it is common to have !1234 used for something else in markdown/text, so that might not be a problem.

However there are other places where # should be replaced with !. What I can think of now is when you do Squash and Merge on a PR, then the PR number is written by default in the heading. This should be replaced (PR title (#1234) -> PR title (!1234)). But there might be persons that doesn't like that change? Should it be a app.ini setting?

@guillep2k
Copy link
Member

However there are other places where # should be replaced with !. What I can think of now is when you do Squash and Merge on a PR, then the PR number is written by default in the heading. This should be replaced (PR title (#1234) -> PR title (!1234)). But there might be persons that doesn't like that change? Should it be a app.ini setting?

Good catch. If the PR I'm working on is accepted, then what we can do is to automatically use the !1234 format when issues are external and numeric, which is the only combination that would make #1234 incorrect for the PR. In all other cases, #1234 will work as expected, independently that !1234 will also work for PRs.

@davidsvantesson
Copy link
Contributor Author

For consistency I think it would be good to have a setting to always use ! instead of # in that case. Because you might start a repository without external tracker and then switch, which would make the old commits incorrect. Also you might have different repos where only one is with external tracker then it makes sense to always use ! for PRs.

@guillep2k
Copy link
Member

For consistency I think it would be good to have a setting to always use ! instead of # in that case. Because you might start a repository without external tracker and then switch, which would make the old commits incorrect. Also you might have different repos where only one is with external tracker then it makes sense to always use ! for PRs.

Mmm.... I'm not so sure about that. The fact that different repos have different needs makes it so an app.ini setting is not appropriate. Then we have all the existing repositories that already use # (and by existing I mean now and at the moment the app.ini setting is changed). Plus users (not admins, which will have access to app.ini) that might hate the change if they don't need it. Finally, if the repo switches between external and internal tracking they will get all the references wrong anyway. 😄

@davidsvantesson
Copy link
Contributor Author

You are probably right app.ini is not suitable for such setting. If needed it can maybe be repo pr unit setting. But that can be discussed once we have the possibility to use ! for pr reference 😄

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants