Hi you all,
beside the discussion of how large code “commitments” or “contributions” should be and which problems they may imply, let me please add the following comment:
Important next to the verifiability of the code and related to this the size of the commitment or contribution is a clean, understandable and complete documentation of the code and/or the changes made as well as their motivation, why this was changed. This makes the control work much easier and makes it much easier for newcomers to get started.
When calculating the code size, the comment portion should be excluded, because otherwise the developer is motivated to keep the code short at the expense of the documentation. This is not a good practice. This is a suggestion for Eclipse.
Furthermore, the code should not only be checked for IP violations, but also whether sufficient unit tests ensure code execution security, which should also be commented.
For the moment, the focus is understandably on the restart of the Birt project. I think that one of the success factors of the Birt project was / is the very good user documentation with its practical examples. If this is affected by the code changes, it should also be maintained.
In short: the technical as well as the user documentation belongs to the source code !
I would be happy if this aspect could be considered after Birt has learned to walk again.
Best
Chris
Von: birt-dev [mailto:birt-dev-bounces@xxxxxxxxxxx] Im Auftrag von Wayne Beaton
Gesendet: Sonntag, 7. März 2021 20:51
An: Alexander Lehmann
Cc: For developers on the BIRT project
Betreff: Re: [birt-dev] Rules for PRs
Yes, the ECA checker that the Eclipse Foundation webmaster installs on our Git repositories checks for both a current ECA and for the signed-off-by requirement. Note that BIRT project committers don't need to add the signed-off-by (I tend to add it to all of my commits just as a matter of practice).
I tend to agree with your comments regarding generated code. However, in practice it tends to be committers that generate the code and the IP Team review requirement doesn't apply to committers doing day-to-day work on a project.
Asking a contributor to break up changes into manageable functionally-complete chunks for review purposes is completely reasonable. Please do not ask contributors to arbitrarily break up contributions to avoid having to deal with the IP Team.
Send a note to emo@eclipse with a pointer to the problematic pull request and I'll have a look.
On Sun, Mar 7, 2021 at 1:40 PM Alexander Lehmann <a.lehm@xxxxxx> wrote:
Hi,
ahh, I mistook the signing that can be enforced by the GitHub branch protection with the "signed off by" that the ECA requires. The latter is then enforced by the ECA-check already?
About the 1KLOC policy: Can you provide a link to that? Couldn't find one by myself. I hope there is an exclusion for generated code since it is not affected by IP rights (at least not in Germany). The example we currently face (one of my pull requests) is such a case (settings files generated by m2e).
For contributions made by a human I totally agree; I prefer not having to review 1000+ lines in one commit and if I face such a case I will request the contributor to break up the changes.
The Eclipse Foundation requires that all contributions be signed off by contributors who have signed the Eclipse Contributor Agreement (that is, they need to include a "Signed-off-by" field in the footer). This is different from signing commits. I'm actually curious to see how requiring that commits be signed actually works in practice. I'm concerned, though, that requiring it would be yet another barrier and we should be all about lowering barriers at this point.
Regarding commit size... Per the IP Policy, any commit of more than 1KLOC needs to be reviewed by the IP Team. I know that there are some committers who encourage contributors to chop up their contributions to keep the commit size down to avoid this requirement. Please don't do this.
Having said all that, IMHO, it is completely unreasonable to expect a committer to review a very large contribution. Further, it is better for everybody, committers and contributors, to push work in incremental chunks (reviewing is easier, and it's easier for others to keep up with the changes). Massive code dumps just make it hard for others to collaborate.
Yes, I think that a review is required for any substantial commit. I think a committer has to request a review. Can this be done in GH?
Signed commits are not needed IMO because of the checks the foundation already requires.
On Sun, Mar 7, 2021 at 9:52 AM Alexander Lehmann <a.lehm@xxxxxx> wrote:
Hi all,
should we agree on some rules for PRs? Alexander F. mentioned for example an upper bound on the LoC per PR. I would like to suggest that a review must be made before a PR can be merged. Should we also require commits to be signed? This can both be accomplished throught a GitHub branch protection rule [1].
_______________________________________________
birt-dev mailing list
birt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/birt-dev
_______________________________________________
birt-dev mailing list
birt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/birt-dev
--
Wayne Beaton
Director of Open Source Projects | Eclipse Foundation
_______________________________________________
birt-dev mailing list
birt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/birt-dev
_______________________________________________
birt-dev mailing list
birt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/birt-dev
--
Wayne Beaton
Director of Open Source Projects | Eclipse Foundation
