go-gitea / gitea

Git with a cup of tea! Painless self-hosted all-in-one software development service, including Git hosting, code review, team collaboration, package registry and CI/CD
https://gitea.com
MIT License
44.89k stars 5.48k forks source link

Scoped label improvements #22974

Open brechtvl opened 1 year ago

brechtvl commented 1 year ago

Feature Description

Now that there is an initial implementation of scoped labels (#22585), there are some things to improve. Aimed at v1.19:

Important (for v1.20?):

Nice to haves:

lafriks commented 1 year ago

Created PR #22976 to allow creating labels from yaml template files that allows exclusive scope option set

lafriks commented 1 year ago

One thing I noticed (could be bug?) - non-exclusive scoped labels should also be rendered same as exclusive

brechtvl commented 1 year ago

Communicating which labels are exclusive seems important. So even if we render non-exclusive ones in a similar style, there should probably be some visual difference still? I don't immediately have a good idea for what that would look like.

lafriks commented 1 year ago

I don't think it's important to see it's exclusive or not when displaying issue or in issue list. It's only important when displaying label selection menu where it's already understandable which are exclusive and which are not

BLumia commented 1 year ago

Quoted from the release note:

This should probably mark as breaking since existing labels may already contain /, e.g. triage | confirmed / verified (we used to use | as delimiter).

Also, could we re-consider making the scope delimiter configurable? A per-instance option could be enough IMO. Some reason:

  1. For existing labels, / can means "or". Using / as scope delimiter makes people not able to use / to express "or" for short.
  2. Being able to set delimiter to something else (e.g. | or ::) could help avoid public services like Codeberg break labels in existing projects https://github.com/go-gitea/gitea/pull/22585#issuecomment-1408550024
  3. To me, :: make perfect sense since it stands for namespace in some programming language and also could avoid such conflict. Also, GitLab already uses that so people don't need to learn it again if they are familiar with GitLab. It might seem ugly if we see that as pain text but we don't need to render it that way...
lafriks commented 1 year ago

As I already mentioned in PR configurable delimiters will make it problematic especially in following cases:

lafriks commented 1 year ago

NB, if you do not explicitly set that this scoped label is explicit only display will be affected not functionality so nothing will break for existing instances

BLumia commented 1 year ago

If instance configuration is changed - all existing labels will break

IMHO it's the instance maintainer's responsibility to keep the label not breaking.

Migrating repository from one instance to other with different configurations will break scoped labels

For the current configuration, migrating issues from GitLab to Gitea might break the scoped labels since the delimiter is different and we cannot change it to the same one if intended to keep labels not breaking.

Same as above applies for federation - will have problems if instances have different delimiter configurations

Honestly, if we insist to keep this unconfigurable, I'll +1 for using :: instead of / which will cause much less issues, but since / already been merged, so I think making it configurable could still help for some small team to avoid breaking scoped labels.

this scoped label is explicit only display will be affected not functionality so nothing will break for existing instances

Existing labels that contain / will display in a confusing way if the user doesn't realize there is a new feature related to label. That's also considered breaking.

lunny commented 1 year ago

Quoted from the release note:

This should probably mark as breaking since existing labels may already contain /, e.g. triage | confirmed / verified (we used to use | as delimiter).

Also, could we re-consider making the scope delimiter configurable? A per-instance option could be enough IMO. Some reason:

1. For existing labels, `/` can means "or". Using `/` as scope delimiter makes people not able to use `/` to express "or" for short.

2. Being able to set delimiter to something else (e.g. `|` or `::`) could help avoid public services like Codeberg break labels in existing projects [Scoped labels #22585 (comment)](https://github.com/go-gitea/gitea/pull/22585#issuecomment-1408550024)

3. To me, `::` make perfect sense since it stands for namespace in some programming language and also could avoid such conflict. Also, GitLab already uses that so people don't need to learn it again if they are familiar with GitLab. It might seem ugly if we see that as pain text but we don't need to render it that way...

I don't think it's a break current implementation? There is a new column exclusive. The labels contains / will be considered scoped only exclusive is true.

BLumia commented 1 year ago

Sorry for not actually trying the feature, this feature looks like this:

image

If user mark it as exclusive then / will be used as scope delimiter. For existing labels, exclusive will not be marked so it won't break anything. So I guess we don't need to mark it as breaking :)

btw, a use-case, do we support escaping the delimiter? For example, the label I mentioned above: triage | confirmed / verified. How can I write this in the current implementation? Is triage/confirmed \/ verified correct?

(p.s. the reason I think :: is good for use as delimiter is, we can safely say escaping is not needed at all since it semantically means namespace and user won't use it for anything other than scoping)

lafriks commented 1 year ago

For the current configuration, migrating issues from GitLab to Gitea might break the scoped labels since the delimiter is different and we cannot change it to the same one if intended to keep labels not breaking.

As we know that GitLab will always have :: as delimiter we should convert them to Gitea delimiter when migrating labels.

BLumia commented 1 year ago

As we know that GitLab will always have :: as delimiter we should convert them to Gitea delimiter when migrating labels.

For example, how it will be if the original label in GitLab is triage::confirmed / verified?

BLumia commented 1 year ago

Another idea:

  1. Still make the delimiter configurable, and use / by default since people may think :: is ugly.
  2. Whatever user inputs in the label form, when they submit the form, replace all the delimiter to :: in database if Exclusive is checked. Which means we always use :: as delimiter in database.
  3. When retrieving label name from database, replace the :: back to the delimiter that user configured.

If we do this, then:

  1. If instance configuration is changed, all existing labels will NOT break since the in-database delimiter won't get changed.
  2. It's then possible to use / as non-delimiter without escaping, so triage::confirmed / verified is possible without doing escaping.
  3. Migrating repository from one instance to other with different configurations will NOT break scoped labels, too.
  4. Users will still able to use / (or whatever they like) as delimiter in the web interface so user won't think it's ugly.
brechtvl commented 1 year ago

Personally my preference was to use :: but I also think the / we ended up with is fine. For either I think it's best for this to not be configurable. I made arguments for this in the original code review and my opinion on this is still the same.

I don't think changing :: just in the database works well, there also the API and any automation like bots built on that to consider. We can't cleanly hide the details of that, to me it seems like that risks further confusion. Having a single simple convention is easier in that sense.

Personally I think it's reasonable that in the (rare) case a label contains / with a different purpose it can be changed to e.g. or, | or something else.

BLumia commented 1 year ago

I don't think changing :: just in the database works well, there also the API and any automation like bots built on that to consider.

Just ensure all API will always get :: and document this behavior. Make the configurable delimiter only works on the frontend instead of backend (consider those APIs are for backend, so always use the raw string that uses :: as delimiter in the API. I guess that will work.

Personally I think it's reasonable that in the (rare) case a label contains / with a different purpose

I honestly don't think the usage of / for 'or' will be rare.

dboreham commented 1 year ago

Hopefully this is a good place to mention: has any work been done on sorting by scoped label value in the issues list? The specific use case I have is to sort on Priority/xxx. This would need some way to assign an order key to each label in the set. I'm interested in implementing this if nobody is working on it.

brechtvl commented 1 year ago

To my knowledge, no work has been done on such sorting.

lafriks commented 1 year ago

Hopefully this is a good place to mention: has any work been done on sorting by scoped label value in the issues list? The specific use case I have is to sort on Priority/xxx. This would need some way to assign an order key to each label in the set. I'm interested in implementing this if nobody is working on it.

Yes, I'm slowly working on this for 3y now 😅 #11669