Review components and keywords, define labels for future use #99
Comments
|
So the migration is a good time to update the components list? Would there be a corresponding (fixed) list in Github? I don't know if we have anything to do with keywords though. They are just whatever the authors chose. |
|
Yes, our script maps components to labels of the form "component: ...." The set of labels is fixed for the repository (can only be changed by privileged users), and it needs to be a small list because the UI is a dropdown list for all labels. (Our prefix "component: " makes sure that they are kept together in the list.)
|
|
Outdated! github labels for components:
|
|
Outdated! Mapping trac components to github labels:
|
|
Maybe, 21 component labels are still too many? |
|
Apart from mapping trac components to github labels, we may want to leave the trac component in the issue's description. |
|
After we decide the maximum number of github component labels, I could request a help on sage-devel. |
|
What is the purpose/meaning of the two-letter code? |
This is definitely a good topic for sage-devel. A brief previous discussion on this happened in https://groups.google.com/g/sage-devel/c/2dKvRwdwVQM/m/dDmGmheFAgAJ |
That is to organize the mapping from trac components to github labels, as a temporary stepping stone. Also it groups the github labels by:
and X for miscellaneous or anything. The second letters were chosen arbitrary. |
|
The number of labels, presently 21, is adequate? Could we increase? |
mkoeppe
changed the title
|
In addition to the label name, we can also play with the label's |
I'll have a version of the imported repository ready later today, then we can check how things feel on the UI side of things |
|
Is there a limit on the length of each label? If I had an issue related to categories, I am not sure that I would realize it should belong under "Foundation" (or "Foundations"), but a longer label might capture that. Similarly, "Geometry & Topology" would be better than "Geometry", especially since topology is not geometry. "Symbolics & Calculus". Does "Refactoring" need to be a label? Re colors: I like the idea of using one color for the M labels, another for I, etc. |
I don't know if there's a hard limit, but probably we shouldn't make it much longer than "component: interfaces: optional" so that the display in the issue list does not become unwieldy |
+1 on merging these two into one |
Probably not |
That would be easy to implement. |
|
This is a bit ugly, but "c: ..." may suffice. Or if we use the colors effectively, then we may not need "component: " prefix at all. |
Good idea. |
Then trac components "refactoring" and "relocation" are mapped to what? XX (miscellaneous)? |
|
We should allow and encourage multiple components: a mathematical issue might fall between algebraic geometry and number theory, and maybe that should be be labeled with "algebra", "number theory", and "geometry & topology". |
|
I agree. That is allowed by default? |
Though "refactoring" is not a component of the software sage, it seems an apt label for issues that restructure the codebase. For example, many issues for sage modularization may get this label. Note that the trac component "refactoring" is ranked high in the list (the trac components are ordered in frequency in trac) |
Similarly with graph theory - is that combinatorics or topology? Probably want that label to be explicit as to where graphs go, since by Matthias' list that had the fourth most tickets (and combinatorics the first!). |
|
How about "discrete math" instead of "combinatorics"? The area of discrete math encompasses both combinatorics and graph theory, and "discrete math" is much shorter than "combinatorics & graph theory". Personally I think graph theory is not a branch of topology though their intimate connection. |
This makes a lot of sense to me and to me even justifies a graph theory category on its own. It seems like a lot to lump two of the four biggest categories together without having the labels lose meaning. |
|
"feature" would be a useful type label, regardless how it was used in trac. |
If I'm still not really convinced that these prefixes add enough value that merits the additional visual clutter in issue lists. But I'd be willing to try a PR on a small test migration to our GHE instance. |
|
(That would have to happen within the next few hours.) |
|
#174, but maybe needs further refinement. |
|
The PR gives |
|
I updated #99 (comment) to reflect the discussion up to now. |
|
Or "x: duplicate" "x: invalid" "x: wontfix" "x: worksforme" instead of "r: duplicate" "r: invalid" "r: wontfix" "r: worksforme" so that the order becomes "s: ", "t: ", "x: " |
|
And |
|
I have to mention one more point on the drawbacks of more prefixes: Search criteria become harder to type in the issue search box. For example, .instead of using |
It's simply the most memorable character after "s", different from "t". |
Yes, that is a drawback of any prefix. It is one of the reasons why I initially didn't like the idea of prefixes. But after we accepted labels like "c: ..." or "p: critical / 2", I don't think about it anymore. (I still don't like " / 2" postfix!) |
mkoeppe
modified the milestones:
Right after the migration,
Modernization of the workflow
Okay, I was not sure about this one. Removed the prefix again. |
|
@sagemath/triage One more change to consider about already beautiful labels, before too late, is to replace "s: " prefix with "z: ". Then the labels are ordered nicely: a typical pr would have labels like Blah blah "c: linear algebra" "p: major / 3" "t: enhancement" "z: needs review" Note that the ordered combination of the last three labels reads well as a sentence :-) |
|
Do we need to keep the |
|
I agree that for pull requests, these labels are obsolete: any open PR which is not a draft is implicitly seeking review. Moreover I recommend discouraging having 'draft' PRs at all. If your branch is not ready for review, don't make a PR. (We don't use draft PRs in the LMFDB.) It is a useful feature that when a PR is created from a user's branch, whenever that branch is updated (after review, say), the PR is automatically updated; but it can be annoying when the PR authors make the PR early while still workon on their branch and repeatedly update the branch and PR. |
|
draft PRs are akin to "needs work", IMHO |
The difference between “new” with code and “needs work” is not clear to me. |
How would you advise to proceed to ask for comments on a version of the code that is not ready for merging (in other words, to ask for comments without asking for approval)? |
|
Perhaps on the issue, where you could link to the branch on your fork? But if people want to use draft PRs for this purpose, I would be willing to try it out. |
|
The main point of the "positive review" label is to signal to the release manager that this PR can be merged. Similarly, "needs review" signals to devs that they should have a look at this PR, while "needs work" says that someone needs to work on this PR to move it forward. This information is there in the form of the "Approved" and "Changes required" state of a PR, but github hides this quite well so that labels can be used to make it clearer who should look at this PR next. There is also the idea to auto-sync the review state to the label: #117. But yeah, leaving of the labels could also work. For me a draft PR is a "hey, I'm working on this. It's not yet ready, but if you want to have a look at it you are welcome to do so." and is handy if you want to prevent that multiple people are working on the same thing. Also normal PRs sometimes go back to draft mode, when there are huge changes required and the author of the PR doesn't have time right now to work on this (or it is blocked by an external dependency etc). |
Does CI run on branches that are simply uploaded to a forked repo? I interpreted opening a draft PR as a way of saying I want a CI run to happen on this PR, but don't want reviews yet until that finishes (at which point I'll convert to a non-draft PR) |
If #117 gets done, then I have nothing against redundant labels. But I already see some confusion about which one should use when reviewing code, and some PRs approved with no positive review label or conversely. So unless it is done soon, my vote is to get rid of the labels :-) |
|
There are also a few edge-cases that make this more tricky. For example, say I want to approve the changes of the PR but would like to have a second pair of eyes on it ("needs review" label + positive review). Or someone disagrees with the implementation but is voted-over by other devs ("positive review" label + one changes-requested review). So maybe for now we add a short guide on when to use which label to https://github.com/sagemath/trac-to-github/blob/master/docs/Migration-Trac-to-Github.md#for-reviewing-a-change? |
I have already started to work on that. See the code on my fork of this repo. |
|
On Tue, 7 Feb 2023, 10:02 Alex J Best, ***@***.***> wrote:
Moreover I recommend discouraging having 'draft' PRs at all. If your
branch is not ready for review, don't make a PR. (We don't use draft PRs in
the LMFDB.) It is a useful feature that when a PR is created from a user's
branch, whenever that branch is updated (after review, say), the PR is
automatically updated; but it can be annoying when the PR authors make the
PR early while still workon on their branch and repeatedly update the
branch and PR.
Does CI run on branches that are simply uploaded to a forked repo, I
interpreted opening a draft PR as a way of saying I want a CI run to happen
on this PR, but don't want reviews yet until that finishes (at which point
I'll convert to a non-draft PR)
you can enable Actions on your fork, and have the CI they provide - which
should be enough for most purposes.
—
… Reply to this email directly, view it on GitHub
<#99 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJXYHDUOKDGS4NGEKVSKFTWWIMT5ANCNFSM6AAAAAATTRXQXY>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
|
Thanks, I was confused by the fact that they were not enabled by default, and that clicking on settings they seem to be enabled. |
I've opened PR #187 for it. |
component_frequency.txt
keyword_frequency.txt
The text was updated successfully, but these errors were encountered: