docs: add contributing guide
ClosedPublic

Authored by romangg on Sep 18 2019, 10:27 PM.

Details

Reviewers
None
Group Reviewers
Plasma
Commits
R104:50cf930ec824: docs: add contributing guide
Summary

This adds a document providing information on how to contribute to KScreen.

It includes clarifications about the used coding style and introduces a Commit
Message Guideline based on the KDE Commit Policy and the Conventional Commits
specification.

Diff Detail

Repository
R104 KScreen
Branch
commitPolicy
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16759
Build 16777: arc lint + arc unit
romangg created this revision.Sep 18 2019, 10:27 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 18 2019, 10:27 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
romangg requested review of this revision.Sep 18 2019, 10:27 PM
romangg updated this revision to Diff 66422.Sep 18 2019, 10:34 PM
  • Add period
  • Change or to and
romangg updated this revision to Diff 66424.Sep 18 2019, 10:41 PM
  • Grammar fix

Is this Conventional Commits spec something we think we want to use elsewhere? I'm sure I can get used to it, but the prefix thing seems kind of rigid and I'm wondering what it really adds other than a bunch of process that will need to be explained over and over again to each patch submitter.

Is this Conventional Commits spec something we think we want to use elsewhere? I'm sure I can get used to it, but the prefix thing seems kind of rigid and I'm wondering what it really adds other than a bunch of process that will need to be explained over and over again to each patch submitter.

As I would interpret it the type is first of course a concise way to tell what the commit is about but it is also there to limit a commit to a single purpose prompting to keep the commit size limited.

The optional scope you can often already see in several KDE projects being used what indicates that this is rather a natural thing to do. Currently the format is just not in a consistent manner. Often box brackets are used (KWin for example) but these lead to problems with certain git tools I heard in the past.

Having the prefix also improves creation of changelogs. I wrote a mail about the overall idea as something to unify in all of KDE little while ago to the mailing lists: https://mail.kde.org/pipermail/kde-frameworks-devel/2019-August/091237.html
So you can see this here as a test run.

In terms of changelogs, you can already use the FEATURE: and CHANGELOG: tags in your commit message. That's already there and similarly we're not actually using it. Might it make sense to develop better documentation around using those throughout all of KDE?

In terms of changelogs, you can already use the FEATURE: and CHANGELOG: tags in your commit message. That's already there and similarly we're not actually using it. Might it make sense to develop better documentation around using those throughout all of KDE?

I think that the FEATURE and CHANGELOG keywords are not often used is not a problem of missing documentation - it is there after all and I linked to it - but that they have issues on a conceptual level:

The FEATURE keyword associates a Bugzilla bug with a commit. But often a new feature is not implemented because of a Bugzilla feature request but there was a Phabricator task for example.
The CHANGELOG keyword is cumbersome to use because the summary line most often already tells sufficiently what the commit is about. So it would only be a repetition of what is already written. Besides why should we want to have a disparity between changelog and commit log anyway?

In comparison using the prefix allows to reuse the subject, organize a changelog in categories and filter out irrelevant commits for example changes to autotests. In a standard case information is not repeated unnecessarily: most often already now one writes something like "Fix the poor behavior in component". Now one writes: "fix(component): improve behavior".

At last the "special keywords" are something only we in KDE use as far as I know, but the Conventional Commits specs, respective the Angular guideline, is something a lot of other projects use and their adaption shows that it seems to work fine at least for them. Also there are sysadmin/ci tools available to make use of it.

The CHANGELOG keyword is cumbersome to use because the summary line most often already tells sufficiently what the commit is about. So it would only be a repetition of what is already written. Besides why should we want to have a disparity between changelog and commit log anyway?

I use the CHANGELOG tag quite often. The difference being that the summary is perhaps something like "Avoid null pointer dereference" with CHANGELOG being a more friendly line for the changelog "Fix crash when doing xyz".

romangg updated this revision to Diff 66437.Sep 19 2019, 9:53 AM
  • Explain types

The CHANGELOG keyword is cumbersome to use because the summary line most often already tells sufficiently what the commit is about. So it would only be a repetition of what is already written. Besides why should we want to have a disparity between changelog and commit log anyway?

I use the CHANGELOG tag quite often. The difference being that the summary is perhaps something like "Avoid null pointer dereference" with CHANGELOG being a more friendly line for the changelog "Fix crash when doing xyz".

I went quickly through KScreen, libkscreen, KWin, KWayland, plasma-desktop and plasma-workspace repositories to check if you're an outlier or if it is more often used than I expected. Results:

RepoCommits w. CHANGELOGLast usageKai Uwe share
KScreen2May 20160%
libkscreen2Sep 20170%
KWin2Mar 2017100%
KWayland1Oct 20160%
plasma-desktop26Jun 201992%
plasma-workspace41Aug 201988%

So I admire your expertness on using the available tools to their fullest potential in contrast to everybody else but the numbers also show that the concept hasn't really caught on. I think a more integrated approach in the commit workflow with the prefix can improve the changelogs also quite much. Maybe this will then also encourage people to use the CHANGELOG keyword more often. Since of course with a common prefix it would still be possible to use the keyword.

Thanks in advance for fixing this minor typo.

CONTRIBUTING.md
59

Typo: "communication" (singular) or "happen"

romangg updated this revision to Diff 68445.Oct 21 2019, 2:49 PM
romangg marked an inline comment as done.
  • Rebase
  • Fix typo
romangg added inline comments.Oct 21 2019, 2:49 PM
CONTRIBUTING.md
59

Thanks!

romangg updated this revision to Diff 68917.Oct 28 2019, 8:50 PM

Remove scopes common and console. Small areas, do not warrant scope on themselves. If there is a change to one of them just don't use the optional scope.

romangg updated this revision to Diff 68918.Oct 28 2019, 9:24 PM

Add a minimal readme file

README is really good.

Did you email kde-devel about this commit policy? I thought you did, but I can't find it.
As someone who regularly commits across many many repos if every maintainer enforced their own thing I would really struggle.

CONTRIBUTING.md
69

What's a BREAKING CHANGE in relation to a UI?

configs breaking and being unusable or just a button having new text?

README is really good.

Did you email kde-devel about this commit policy? I thought you did, but I can't find it.
As someone who regularly commits across many many repos if every maintainer enforced their own thing I would really struggle.

Thanks, I have emailed about the general policy this one is based on. Since there is no other one at the moment I don't see the need to ask again. It would be a rather theoretical discussion. Should a second one come around we can align them then.

CONTRIBUTING.md
69

I understand it only for API or config breaks, not behavioral changes if there is a clean migration path and the user just have to learn a new workflow.

Since I felt it to be somewhat vague or maybe not applicable to all our apps I made it less formal here and only recommend it as prose in the commit body.

mart added a subscriber: mart.Nov 5 2019, 9:31 AM

Overall most of those are sensible policies (and would really make sense globally in plasma, rather than a specific subproject)
I feel the Conventional Commit policy to rise the barrier of entry a tad too much tough (and not sure i would really want it in the resto of plasma)

i think something along those lines would really make sense globally in plasma, having different sub projects having different policies i feel it may be confusing to the new contributor... tough, your call, just a personal feeling :)

In D24068#558809, @mart wrote:

Overall most of those are sensible policies (and would really make sense globally in plasma, rather than a specific subproject)
I feel the Conventional Commit policy to rise the barrier of entry a tad too much tough (and not sure i would really want it in the resto of plasma)

My plan is for new contributors to just edit their Phabricator revision title myself such that the conventional commit policy is respected in this regard. In general I tried to stay as close as possible to the conventional commit policy just so we can talk to them in the future without being a fork. Also I think it's easier to relax conditions later on than to introduce stricter ones.

i think something along those lines would really make sense globally in plasma, having different sub projects having different policies i feel it may be confusing to the new contributor... tough, your call, just a personal feeling :)

Sue, we should avoid different policies. KScreen/libkscreen are kind of isolated on what they do so I think we can go ahead try the policy and either scrap it later if it's not working well or discuss after analyzing the usefulness in KScreen/libkscreen in which form we want to expand its usage.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2019, 8:52 PM
This revision was automatically updated to reflect the committed changes.

Since there is no other one at the moment I don't see the need to ask again

You could at least follow up the replies. It was even met with a positive response, with some effort this could easily become a thing.

It's a completely different story to say kscreen would become an early adopter of an agreed upon new standard we're adopting generically to saying we're going to be a special unique snowflake on one tiny subpart of a bigger project.

David F also had a really good comment about the "testing done" section, that's worth folding into the example.