Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update contribution documents #8382

Merged
merged 6 commits into from
Aug 16, 2021
Merged

Update contribution documents #8382

merged 6 commits into from
Aug 16, 2021

Conversation

kymckay
Copy link
Member

@kymckay kymckay commented Aug 16, 2021

When merged this pull request will:

  • Remove component maintainers from readme files. Source of confusion and entirely unused.
  • Reflect our actual PR merging process.
  • Reflect our actual attribution practices.
  • Update coding guidelines to reflect changes to function include position

- Reflect true attribution practices
- Reflect true merge process (this changed a long time ago)
We have almost never used these and serve as a source of confusion for
new contributors.
Caught in a bad regex
Comment on lines +174 to +176
Every function includes the `script_component.hpp` file on the first line. Any additional includes or defines must be below this include.

All code written must be below this include and any potential additional includes or defines.
Copy link
Member

@jonpas jonpas Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional includes or defines

Duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same, but think it's just badly worded. Code must appear below all includes and defines.

Includes and defines must appear below the script component include.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole document could probably use a once over separately. I just spotted the outdated info while doing this. 👍

docs/wiki/development/merging-pull-requests.md Outdated Show resolved Hide resolved
docs/wiki/development/merging-pull-requests.md Outdated Show resolved Hide resolved
@kymckay kymckay merged commit 5612359 into master Aug 16, 2021
@kymckay kymckay deleted the kymckay/doc branch August 16, 2021 19:17
The people responsible for merging changes to existing addons are the maintainers listed in the README.md file of the respective addon folder.
#### Merge Criteria

All pull requests must receive approval from a maintainer and must pass all required continuous integration checks before they can be merged.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be members instead of maintainers now?


#### New Addons / Other Changes
If a pull request adds a new addon, or changes something else, like the README, everyone has 72 hours to comment on the changes. After that, one of the project leads will merge it.
Non-trivial pull requests should ideally be thouroughly reviewed by multiple maintainers or at least one maintainer highly familiar with any code modified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

LinkIsGrim pushed a commit to LinkIsGrim/ACE3 that referenced this pull request Aug 28, 2021
* Update code guidelines for script_component

* Update contributing documents for pull requests

- Reflect true attribution practices
- Reflect true merge process (this changed a long time ago)

* Remove listed maintainer from component readmes

We have almost never used these and serve as a source of confusion for
new contributors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants