search

KiCad / kicad-symbols

Official KiCad schematic symbol libraries for Kicad 5
https://kicad.github.io/symbols
Other
707 stars 746 forks source link

Organization of labels (Question to the Librarians) #1368

Open myfreescalewebpage opened 5 years ago

myfreescalewebpage commented 5 years ago

Hi Librarians !

That's some few weeks ago I have the pleasure to be in the team, and I would like to propose you some cleanup of the labels we use to review the PR on the three main repository: symbols, footprints and packages 3D.

Today the labels are not all very clearly defined, some are defined twice etc. I have collected all of them and made a small status. Here is my proposition below.

Very useful labels we clearly need and we need to keep in the three repositories:

One new label I propose to mirror the pending footprint label in the footprints repository:

All the other are below and my main position is that most of them are not useful and should be removed (there are too much labels!):

Please let me known what you think about this.

This is a question to librarians so Question and Librarian labels applied :)

Cheers, Joel

fauxpark commented 5 years ago

"Good first issue" I assume means that the issue is easy enough for someone new to the libraries to make it their first contribution. Perhaps it should be better documented.

myfreescalewebpage commented 5 years ago

"Good first issue" I assume means that the issue is easy enough for someone new to the libraries to make it their first contribution. Perhaps it should be better documented.

Without making the review first, this is sometimes a little bit tricky to say if it is a good issue to start or not

poeschlr commented 5 years ago

Good first issue is a github default label. The same is true for help wanted and bug.

myfreescalewebpage commented 5 years ago

Good first issue is a github default label. The same is true for help wanted and bug.

Right, but purely some examples provided by GitHub that you can remove/modify.

evanshultz commented 5 years ago

Why keep Scripted alternative available? If this is true, and another PR exists as mentioned above, just close the unscripted one (make sure there's a link between the PRs).

Rather than prebooked we need only use Booked for proper English and brevity. Alternatively, the milestone tool could be used and this label removed. I prefer the milestone method personally.

I think Good first issue and Help wanted are useful for catching low-hanging fruit. I can point to specific examples where contributors jumped on these issues quickly after I created them. And it's also a way to sweep up some easy issues that we may want to address before or after a release. Maybe we don't need both, but I think one is useful.

IMO we definitely do need a KLC issue label for when some discussion comes up in a PR or an issue revolves around a possible change to KLC. If we don't have a way to scoop up KLC stuff it will too easily get lost.

Why remove Pending changes? If the flow is to submit a PR, and then find a reviewer, after the review there may be requested changes and this label fits. Otherwise there is no categorization, right? And once the changes are submitted, perhaps a Pending review label is useful?

How about adding a Merge Conflict label? This is sometimes rather useful when one merge breaks several other things. It would be nice if we could get this for an entire repo's PRs automatically...

Since you seem to be eager to use and define labels, why not write out a guide how you think they would work? Some only apply to 3D models, for example. Or some to issues and not to PRs. Perhaps once the dust has settled a bit above you can propose label usage.

myfreescalewebpage commented 5 years ago

Why keep Scripted alternative available? If this is true, and another PR exists as mentioned above, just close the unscripted one (make sure there's a link between the PRs).

Agree with you.

Rather than prebooked we need only use Booked for proper English and brevity. Alternatively, the milestone tool could be used and this label removed. I prefer the milestone method personally.

Agree with you. Milestone is the perfect tool for that.

I think Good first issue and Help wanted are useful for catching low-hanging fruit. I can point to specific examples where contributors jumped on these issues quickly after I created them. And it's also a way to sweep up some easy issues that we may want to address before or after a release. Maybe we don't need both, but I think one is useful.

Example is welcome, but regarding to "easy to solve" issues/pulls, I realize sometimes we think it will be easy but it is not so much because the contributor is busy, is not experimented with the KLC or because of various questions about the submission before merging etc.

IMO we definitely do need a KLC issue label for when some discussion comes up in a PR or an issue revolves around a possible change to KLC. If we don't have a way to scoop up KLC stuff it will too easily get lost.

I understand you associate this label to KLC incompatibilities or change required, not "this PR violate KLC". In this case I agree this label is useful.

Why remove Pending changes? If the flow is to submit a PR, and then find a reviewer, after the review there may be requested changes and this label fits. Otherwise there is no categorization, right? And once the changes are submitted, perhaps a Pending review label is useful?

The submitter is not able to remove the "Pending changes" and put back the "Pending review" himself. So he is making changes, but the label stays. My position is that the reviewer assigned is responsible of checking the progress of the pull (he has GitHub notifications when changes are made - if not time to check immediately, option "saved for later" mark the notification so it is not clear), sometimes submitter has not the time to submit all changes required in one time, in this case we can make only a partial second review etc. I personally don't use the "Pending changes" label, I prefer checkbox to list the changes required, and I check when I review them (look at my merged PR if you want). However, we have not all exactly the same way to work to perform the review, and I agree to keep this labels if some reviewers prefer using them.

How about adding a Merge Conflict label? This is sometimes rather useful when one merge breaks several other things. It would be nice if we could get this for an entire repo's PRs automatically...

I don't think there is an automatic way to achieve this. At the opposite of the previous comment, here it is the submitter which is responsible of the submission and have to check the submission he made. However, if an automatic way to indicate this on PR is available, that's a good idea !

Since you seem to be eager to use and define labels, why not write out a guide how you think they would work? Some only apply to 3D models, for example. Or some to issues and not to PRs. Perhaps once the dust has settled a bit above you can propose label usage.

That's why I have submitted this Question to get a first feedback about this. @antoniovazquezblanco worked also on this I think, probably we should write something, I agree to start a doc proposing a kind of workflow. @antoniovazquezblanco also if you have written somethings, that's good to share here. We all have the possibility to interact after on this basis.

Joel

antoniovazquezblanco commented 5 years ago

I have also assigned myself to get back to this as soon as I find some time. Sorry for being away for the last couple of weeks

myfreescalewebpage commented 5 years ago

No worries @antoniovazquezblanco :)

myfreescalewebpage commented 5 years ago

Attached is the workflow I propose, with labels used in the legend, for Issues and PRs.

workflow_issue.pdf workflow_legend.pdf workflow_pull.pdf

This is a proposition to be discussed, let me know your positions :)

Joel

antoniovazquezblanco commented 5 years ago

Just to simplify I would ask about the difference between bug and enhancement. Up until now I was classifying thins as follows:

Maybe those two can become one tag.

myfreescalewebpage commented 5 years ago

I think to the following definition:

antoniovazquezblanco commented 5 years ago

Works for me :).

I would also vote to drop the librarian tag. Whenever needed, one can use the @KiCad/librarians mention to call for help at any time. That should simplify the number of tags.

myfreescalewebpage commented 5 years ago

Whenever needed, one can use the @KiCad/librarians mention to call for help at any time. That should simplify the number of tags.

that's right

myfreescalewebpage commented 5 years ago

Reducing the number of tags is also a key to be more accurate and remove confusion.

evanshultz commented 5 years ago

Note that the librarian tag above hits a whole lot of people, some who are not actively participating in the librarian activities.

myfreescalewebpage commented 5 years ago

@evanshultz just looked at the librarian team, yes seems this is all librarians, not only "experienced" librarians I would like to point in my workflow. I am in the team but do not think I am enough experimented to answer every thing.

By "experienced" librarians (the tag) I include today @poeschlr and you @evanshultz when looking at the number of PR you are assigned (but it depends of the subject of course, some can also be considered experienced on specific subjects! I don't want to get angry with librarians :) )

myfreescalewebpage commented 5 years ago

@poeschlr @evanshultz have you looked at the PDF I have attached ? Your opinion is important for me.

DanSGiesbrecht commented 5 years ago

+1 to Evan's comment. Note that @antoniovazquezblanco added ` ` around the tag: @KiCad\librarians, to prevent it from notifying everyone.

DanSGiesbrecht commented 5 years ago

A few comments from me:

myfreescalewebpage commented 5 years ago

@DanSGiesbrecht

myfreescalewebpage commented 5 years ago

@DanSGiesbrecht up ?

myfreescalewebpage commented 5 years ago

@KiCad/librarians any other comment ? How have you worked on the past weeks ? What do you think of this workflow ?

poeschlr commented 5 years ago

I use the labels as follow: When i review a pull request i assign myself and remove the pending reviewer tag. If the PR can be merged then i merge right away with documenting everything as required (adding screenshots of dimensioned drawings)

If i find a problem then i document what needs to be done and add the pending changes tag. This label is there to communicate the future self and any other librarian that there is an open issue with the pull request. Github does communicate unread changes when i check the list of pull requests assigned to me. (blue mark on the left side of the line.) I then check what changed. If the change is small enough to review right away then i will make an imediate decission if the pull request is now good to go or if it still needs further changes. If the change is too large for checking it with my currently available time then i mark the pull request with "ready for review". (Github would no longer show the blue mark as i looked at the changes. So without a special tag i would not be able to tell which pull request might have its issues fixed already.)

If i then later have time for kicad stuff i will simply check the pull requests marked as "ready for review" first. The pull request is then either merged or if further issues are found marked as pending changes and the requested changes are documented.

The KLC issue label is mainly to mark open issues not to mark pull requests. (See it similar as a prioritization for issues.)

Help wanted and good first issue are github default labels. I don't think we should remove them. both are again mainly meant for marking issues.

There is another label i use quite often. The "scripting required label". It is meant to tell me and other maintainers that a particular contribution is only accepted if it is scripted.

I use this if i have time to script something. (I go through these open pull requests from time to time and simply add them to the script my self if it has been long enough or if i work on the script anyways.)

We might either need a Question by librarian to contributor label or change the description of the current question label to mean "there is an open question by anybody involved"

Misca1234 commented 5 years ago

--- comment on workflow

It will be difficult to implement, maintain and enforce a process for this if it include more than 2 steps. When an item is "in" there is no maintenance of it (as it is almost always is with software) so there is no need for a process to secure easy future maintenance.

Each item (foot print, 3D models etc.) is pretty easy to handle individual and don't depend on other items so attempts to catching obvious errors through a process will not be fruitfull.

People do not show up on daily basis, people is added and others leave for longer times so it is difficult to maintain a process over time, especially if we (and most certainly will) change the process over time.

In some sense the contributors have to understand and therefore follow an establish process and that might be a very large obstacle to handle, especially for those one shoot contributers

--- how I work

I do not see any push as "somone's", but if a librarian take a particular interest in a push I usually leave it alone. "a particular interest" is more than just make some initial comments to the contributor.

If a librarian starts to engage more than normal in a push that I started to comment I usually leave it over to that librarian.

If a push do not get response from the contribute for a long time, I might take it over if it is a fun one, that is, re push it with my login or just delete it.

The flags, which I do not use much are "Await foot print push", for the 3D model and symbol "question", usually aimed to the contributor

--- If you want a break for reviews Remake handmade footprints with scripts instead Make 3D models for the following foot prints. (you can leave battery holder and valves to me, I have special interest them ;) )

slask_2.txt

myfreescalewebpage commented 5 years ago

@poeschlr I have seen how you were working these days ;) Note: there is also a "Saved for later" mark you can add to a notification, same as "ready for review", but I'm not opposed to keep this tag too, the reviewer can use the way he wants.

@Misca1234 I do not understand your comments. The discussion I have started is because the review process is not clear, and first for the submitters. When I was not a librarian, I wan't understanding at all how it was managed, and all reviewers have a different process to do it. The idea is clearly to document this a little bit to provide the main guidelines for the reviews. Several comments on the end of your post:

Following return of @poeschlr , PDF updated below: workflow_issue.pdf workflow_pull.pdf workflow_legend.pdf

If we agreed for such list of labels, I can make the modifications during the weekend.

Joel

Misca1234 commented 5 years ago

@myfreescalewebpage Then I reformulate it in another way

--- comment on workflow You asked for comments, I reformulate it

-My opinion is that a elaborated/complicated/many steps/flow chart process will not work, nor for us or one, few contributers

--- how I work You asked

@KiCad/librarians any other comment ? How have you worked on the past weeks ? What do you think of this workflow ?

This was my answer to that question

You asked

@KiCad/librarians any other comment ? How have you worked on the past weeks ? What do you think of this workflow ?

See Above

--- If you want a break for reviews

Irrelevant for the thread discussions but a tip for people , comment was fairly innocent and can not be considered as an attempt to hijack thread

myfreescalewebpage commented 5 years ago

My opinion is that a elaborated/complicated/many steps/flow chart process will not work, nor for us or one, few contributers

You are free to submit something too, currently I don't know how to simplify my diagrams. Do you agree a process is required at this moment (any way it is) ?

Irrelevant for the thread discussions but a tip for people , comment was fairly innocent and can not be considered as an attempt to hijack thread

Well I'm not afraid of hijacked of the thread, the title is enough clear to know what we are speaking about here. I encourage you to put your txt file in a new issue at https://github.com/KiCad/kicad-packages3D/issues, it's the best place for it and you will be able to maintain it easily, I see you are very good at 3D creation and that's nice :)