Comments on the Code of Conduct #9738
Conversation
|
|
||
| ### Project administration | ||
|
|
||
| - The project founders MUST act as Administrators to manage the set of project Maintainers. |
There was a problem hiding this comment.
Who are the "project founders"? Should this be the "the Monero Core Team"? Do we have a list of Core Team members? Will the Core Team continue to exist?
There was a problem hiding this comment.
Project founders should be replaced with core team, as they have the permission to add people with write access.
| - Maintainers SHOULD NOT merge their own patches except in exceptional cases, such as non-responsiveness from other Maintainers for an extended period (more than 30 days) or unless urgent as defined by the Monero Maintainers Team. | ||
| - Maintainers MUST NOT make value judgments on correct patches unless the Maintainer (as may happen in rare circumstances) is a core code developer. | ||
| - Maintainers MUST NOT merge pull requests in less than 168 hours (1 week) unless deemed urgent by at least 2 people from the Monero Maintainer Team. | ||
| - The Contributor MAY tag an issue as "Ready" after making a pull request for the issue. |
There was a problem hiding this comment.
I don't know what 'tag[ging] an issue as "Ready"' is supposed to mean. Should we remove this?
| - To discuss a patch, people MAY comment on the Platform pull request, on the commit, or elsewhere. | ||
| - To accept or reject a patch, a Maintainer MUST use the Platform interface. | ||
| - Maintainers SHOULD NOT merge their own patches except in exceptional cases, such as non-responsiveness from other Maintainers for an extended period (more than 30 days) or unless urgent as defined by the Monero Maintainers Team. | ||
| - Maintainers MUST NOT make value judgments on correct patches unless the Maintainer (as may happen in rare circumstances) is a core code developer. |
There was a problem hiding this comment.
Who is "a core code developer"? Should this be "a member of the Monero Core Team"?
Why wouldn't Maintainers be allowed to make value judgments on correct patches?
There was a problem hiding this comment.
I would remove this section. Maintainers should be allowed to make value judgement.
| - To submit a patch, a Contributor MUST create a Platform pull request back to the project. | ||
| - A Contributor MUST NOT commit changes directly to the project. | ||
| - To discuss a patch, people MAY comment on the Platform pull request, on the commit, or elsewhere. | ||
| - To accept or reject a patch, a Maintainer MUST use the Platform interface. |
There was a problem hiding this comment.
We haven't formalized what "rejecting" a patch means. Typically PRs that were NACKd by at least one contributor don't get merged.
See also: #9734
| - Users MUST NOT log feature requests, ideas, or suggestions unrelated to Monero code or Monero's dependency code or Monero's potential/future dependency code or research which successfully implements Monero. | ||
| - Users MUST NOT log any solutions to problems (verifiable or hypothetical) of which are not explicitly documented and/or not provable and/or cannot be reasonably proven. |
There was a problem hiding this comment.
Does 'log' mean 'open an issue for'?
What behavior is this line trying to prevent?
|
|
||
| The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119. | ||
|
|
||
| The "Monero Maintainer Team" is defined in this document as the following users: |
There was a problem hiding this comment.
This list is out of date and should either be removed or actualized. See #9735.
| - All contributions to the project source code ("patches") MUST use the same license as the project. | ||
| - All patches are owned by their authors. There MUST NOT be any copyright assignment process. | ||
| - The copyrights in the project MUST be owned collectively by all its Contributors. | ||
| - Each Contributor MUST be responsible for identifying themselves in the project Contributor list. |
There was a problem hiding this comment.
This implies the existence of a separate "Contributor list" which must be maintained.
We can obtain such a list from git by using: git log --format='%aN <%aE>' | sort -u
I think we can remove this line.
There was a problem hiding this comment.
Yes, any list will quickly be outdated and unmaintained. Remove it.
| - Thus, the release history of the project MUST be a list of meaningful issues logged and solved. | ||
| - To work on an issue, a Contributor MUST fork the project repository and then work on their forked repository. | ||
| - To submit a patch, a Contributor MUST create a Platform pull request back to the project. | ||
| - A Contributor MUST NOT commit changes directly to the project. |
There was a problem hiding this comment.
| - A Contributor MUST NOT commit changes directly to the project. | |
| - Patches MUST NOT be committed directly to the project. |
There was a problem hiding this comment.
A contributor does not have write access anyway, so the above point seems redundant.
| - To accept or reject a patch, a Maintainer MUST use the Platform interface. | ||
| - Maintainers SHOULD NOT merge their own patches except in exceptional cases, such as non-responsiveness from other Maintainers for an extended period (more than 30 days) or unless urgent as defined by the Monero Maintainers Team. | ||
| - Maintainers MUST NOT make value judgments on correct patches unless the Maintainer (as may happen in rare circumstances) is a core code developer. | ||
| - Maintainers MUST NOT merge pull requests in less than 168 hours (1 week) unless deemed urgent by at least 2 people from the Monero Maintainer Team. |
There was a problem hiding this comment.
Is there a good reason to delay a merge by up to one week if the PR has gained sufficient approvals? Would it not make more sense to simply gate merges by some number of approvals from contributors?
Less-than-trivial changes naturally take more time to review.
There was a problem hiding this comment.
I think this should be kept in so that people can get a chance to add their opinion to proposed changes. For pure CI changes it can be skipped IMO.
There was a problem hiding this comment.
The sentence, as written, makes no sense, as it would mean the act of merging itself cannot take less than 168 hours. Moreover, would the 168 hour delay apply from the time of submitting the patch, or gaining the requisite approvals? In any case, I don't like the idea, as it incentivizes larger PRs purely for the sake of avoiding lengthy merge delays.
There was a problem hiding this comment.
Regardless of the exact interpretation of this policy, I dislike it: Assuming an even marginally effective review by those who sign off on the merge, the merged code should net improve the software. Delaying an improvement on the prospect that someone might later object to it doesn't make sense: A patch, or comment, subsequent to the merge can correct any perceived deficiency. And if one objects to the assumption of competent reviewers, then the merge delay may as well be zero, because the review would not improve the work.
| - Maintainers MUST have a Platform account and SHOULD use their real names or a well-known alias. | ||
| - Contributors SHOULD have a Platform account and MAY use their real names or a well-known alias. |
There was a problem hiding this comment.
| - Maintainers MUST have a Platform account and SHOULD use their real names or a well-known alias. | |
| - Contributors SHOULD have a Platform account and MAY use their real names or a well-known alias. | |
| - Maintainers MUST have a Platform account. | |
| - Contributors SHOULD have a Platform account. |
I don't think we should mention/encourage using real names. What constitutes a "well-known alias" is not defined.
|
@x64x2 Can you clarify what you mean with this? |
| ### Development process | ||
|
|
||
| - Change on the project MUST be governed by the pattern of accurately identifying problems and applying minimal, accurate solutions to these problems. | ||
| - To request changes, a user SHOULD log an issue on the project Platform issue tracker. |
There was a problem hiding this comment.
| - To request changes, a user SHOULD log an issue on the project Platform issue tracker. | |
| - To request changes, a user SHOULD open an issue on the project Platform issue tracker. |
| - The user or Contributor SHOULD seek consensus on the accuracy of their observation, and the value of solving the problem. | ||
| - Users MUST NOT log feature requests, ideas, or suggestions unrelated to Monero code or Monero's dependency code or Monero's potential/future dependency code or research which successfully implements Monero. | ||
| - Users MUST NOT log any solutions to problems (verifiable or hypothetical) of which are not explicitly documented and/or not provable and/or cannot be reasonably proven. | ||
| - Thus, the release history of the project MUST be a list of meaningful issues logged and solved. |
There was a problem hiding this comment.
What is "release history"? Is this talking about the release changelog? We don't format our changelog like this.
There was a problem hiding this comment.
I would say remove the sentence.
| - The project MUST use a share-alike license, such as BSD-3, the GPLv3 or a variant thereof (LGPL, AGPL), or the MPLv2. | ||
| - All contributions to the project source code ("patches") MUST use the same license as the project. | ||
| - All patches are owned by their authors. There MUST NOT be any copyright assignment process. | ||
| - The copyrights in the project MUST be owned collectively by all its Contributors. |
There was a problem hiding this comment.
I think we should remove this line because copyright law doesn't work like this.
| - To request changes, a user SHOULD log an issue on the project Platform issue tracker. | ||
| - The user or Contributor SHOULD write the issue by describing the problem they face or observe. | ||
| - The user or Contributor SHOULD seek consensus on the accuracy of their observation, and the value of solving the problem. | ||
| - Users MUST NOT log feature requests, ideas, or suggestions unrelated to Monero code or Monero's dependency code or Monero's potential/future dependency code or research which successfully implements Monero. |
There was a problem hiding this comment.
This is an overly verbose way to say "Issues submitted to the Platform issue tracker MUST be related to Monero". This is common sense.
There was a problem hiding this comment.
It is common sense, though filing irrelevant issues definitely happens, so I can see the rationale. That said, I have no idea how including such a sentence, or not, in any way deters random, unrelated issues from being created (it hasn't up to this point).
| - A patch MUST adhere to the code style guidelines of the project if these are defined. | ||
| - A patch MUST adhere to the "Evolution of Public Contracts" guidelines defined below. | ||
| - A patch MUST NOT include non-trivial code from other projects unless the Contributor is the original author of that code. | ||
| - A patch MUST compile cleanly and pass project self-tests on at least the principle target platform. |
There was a problem hiding this comment.
"self-tests" isn't language I have seen used in this project. The "principle target platform" is not defined.
We automate checking if a patch "compiles cleanly" and passes tests with GitHub actions.
| - A patch MUST compile cleanly and pass project self-tests on at least the principle target platform. | |
| - A patch MUST pass Platform continuous integration checks. |
| - Old names SHOULD be deprecated in a systematic fashion by marking new names as "experimental" until they are stable, then marking the old names as "deprecated". | ||
| - When sufficient time has passed, old deprecated names SHOULD be marked "legacy" and eventually removed. | ||
| - Old names MUST NOT be reused by new features. | ||
| - When old names are removed, their implementations MUST provoke an exception (assertion) if used by applications. |
There was a problem hiding this comment.
If an old name is removed, its implementation can not "provoke an exception" because its implementation no longer exists.
Removing a name will either naturally result in a build error for consumers of our libraries, or an return an error message for users of our RPCs.
There was a problem hiding this comment.
I think I see the intent of these two bullets, though I'm not sure I agree with it:
- Old names MUST NOT be reused by new features.
- When old names are removed, their implementations MUST provoke an exception (assertion) if used by applications.
In short: You can't reuse old names, and trying to do so triggers an error. Removing them by itself would cause references to fail, but then, if a new use of the old name later occurs, the builds would pass, potentially breaking downstream consumers.
| ## Goals | ||
|
|
||
| C4 is meant to provide a reusable optimal collaboration model for open source software projects. It has these specific goals: | ||
|
|
||
| - To maximize the scale and diversity of the community around a project, by reducing the friction for new Contributors and creating a scaled participation model with strong positive feedbacks; | ||
| - To relieve dependencies on key individuals by separating different skill sets so that there is a larger pool of competence in any required domain; | ||
| - To allow the project to develop faster and more accurately, by increasing the diversity of the decision making process; | ||
| - To support the natural life cycle of project versions from experimental through to stable, by allowing safe experimentation, rapid failure, and isolation of stable code; | ||
| - To reduce the internal complexity of project repositories, thus making it easier for Contributors to participate and reducing the scope for error; | ||
| - To enforce collective ownership of the project, which increases economic incentive to Contributors and reduces the risk of hijack by hostile entities. |
There was a problem hiding this comment.
This entire section outlines goals for the original C4 document, but does not necessarily align with our changes. It's also vaguely formulated.
I think we should just remove this because it adds nothing of value.
There was a problem hiding this comment.
This seems like it's targeted to users of C4 ("here's why this was written"); I don't see why it should be included by a downstream user of the document.
| - To work on an issue, a Contributor MUST fork the project repository and then work on their forked repository. | ||
| - To submit a patch, a Contributor MUST create a Platform pull request back to the project. |
There was a problem hiding this comment.
I think the intent of these two lines can be read in two ways:
- "Patches MUST be submitted through Platform pull requests"
- "Patches MUST NOT be submitted to the project by proxy"
I agree with the first interpretation. I disagree with the second. It is acceptable to send another Contributor a patch and have them create a pull request on your behalf. This does not necessarily involve "forking" the repository.
| - The Platform issue tracker would be Trac. | ||
| - The project SHOULD have clearly documented guidelines for code style. | ||
| - A "Contributor" is a person who wishes to provide a patch, being a set of commits that solve some clearly identified problem. | ||
| - A "Maintainer" is a person who merges patches to the project. Maintainers are not developers; their job is to enforce process. |
There was a problem hiding this comment.
| - A "Maintainer" is a person who merges patches to the project. Maintainers are not developers; their job is to enforce process. | |
| - A "Maintainer" is a person who merges patches to the project. |
This may be misinterpreted to mean that maintainers are not allowed to submit patches.
This clearly isn't the intent of the original document, given that it contains:
- "Contributors SHALL NOT have commit access to the repository unless they are also Maintainers."
- "A new Contributor who makes a correct patch SHALL be invited to become a Maintainer."
- "To enforce collective ownership of the project, which increases economic incentive to Contributors and reduces the risk of hijack by hostile entities."
The Code of Conduct was introduced in #2174 and lives at
docs/CONTRIBUTING.md. It was copied from ZeroMQ RFC 22, with miscellaneous changes.This is an effort to re-evaluate and contemporize it.