Skip to content

Reconcile/clarify contribution & development docs#7662

Merged
cramforce merged 3 commits intoampproject:masterfrom
mrjoro:clean-developing
Feb 22, 2017
Merged

Reconcile/clarify contribution & development docs#7662
cramforce merged 3 commits intoampproject:masterfrom
mrjoro:clean-developing

Conversation

@mrjoro
Copy link
Copy Markdown
Member

@mrjoro mrjoro commented Feb 18, 2017

Makes a few changes to reconcile various contribution/development docs.

  • CONTRIBUTING.md

    • reorganized the doc to flow more naturally
    • added a new section for new contributors, pointing them to the other resources (e2e guide, Great First Issues, how to get help)
    • clarified that the CLA should be signed by the same address used in Git commits
    • added additional details to design review section (e.g. license text + followup)

  • DEVELOPING.md

    • clarified that this doc is a reference and serves more advanced cases
    • added an intro section that points to other docs instead of repeating them
    • added additional details to the Sauce Labs section

  • README.md

    • minor change to point only to CONTRIBUTING.md (removing reference to DEVELOPING.md) since CONTRIBUTING is a better entry point for new contributors

This fixes Issue #7569.

cc @bpaduch @pbakaus @camelburrito

@mrjoro mrjoro added this to the Docs Updates milestone Feb 18, 2017
@mrjoro mrjoro self-assigned this Feb 18, 2017
@mrjoro mrjoro requested a review from cramforce February 18, 2017 19:23
cramforce
cramforce requested changes Feb 20, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

Nice, a few comments.

CONTRIBUTING.md
We hope you'll become an ongoing participant in our open source community but we also welcome one-off contributions for the issues you're particularly passionate about.

### Filing Issues
## Filing issues
Copy link
Copy Markdown
Member

@cramforce cramforce Feb 20, 2017

Choose a reason for hiding this comment

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

❤️

CONTRIBUTING.md
The AMP Project is meant to evolve with feedback. The project and its users appreciate your thoughts on ways to improve the design or features.

**Bugs**
To make a suggestion [file an issue](https://github.com/ampproject/amphtml/issues/new) and add the [Type: Feature Request](https://github.com/ampproject/amphtml/labels/Type%3A%20Feature%20Request) label.
Copy link
Copy Markdown
Member

@cramforce cramforce Feb 20, 2017

Choose a reason for hiding this comment

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

They (sadly) can't actually add labels.

Copy link
Copy Markdown
Member Author

@mrjoro mrjoro Feb 20, 2017

Choose a reason for hiding this comment

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

The old version said "use the enhancement tag"; since I can't find a reference to that anywhere else I assumed it was a label. Is there some other way for filers to indicate that it's a feature request, or should we just leave this part of the instructions out?

Copy link
Copy Markdown
Member Author

@mrjoro mrjoro Feb 22, 2017

Choose a reason for hiding this comment

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

I went ahead and removed adding the label altogether to avoid confusing it. I'm assuming we'll be able to tell from context that it's an Intent to Implement and someone with permission to do so will add the tag (onduty, etc.)

Copy link
Copy Markdown
Member

@cramforce cramforce Feb 22, 2017

Choose a reason for hiding this comment

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

Yes, good call.

CONTRIBUTING.md Outdated
* Familiarize yourself with the [AMP Design Principles](DESIGN_PRINCIPLES.md)
* [Create a new GitHub issue](https://github.com/ampproject/amphtml/issues/new) with the [Intent to Implement label](https://github.com/ampproject/amphtml/labels/INTENT%20TO%20IMPLEMENT) to start discussion of the new feature.
* Consider bringing the eng design for your feature to our [weekly design review](#weekly-design-review).
* Before starting on the code for your feature, get approval from the [Tech Lead and a core contributor](GOVERNANCE.md).
Copy link
Copy Markdown
Member

@cramforce cramforce Feb 20, 2017

Choose a reason for hiding this comment

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

Working on updating that page, but I'd prefer if we inline the "Intent to implement" steps here instead of pointing at the legalese.

Copy link
Copy Markdown
Member Author

@mrjoro mrjoro Feb 20, 2017

Choose a reason for hiding this comment

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

What extra steps would we put here? I was linking to that doc so they'd have a list of names to contact.

Copy link
Copy Markdown
Member

@cramforce cramforce Feb 21, 2017

Choose a reason for hiding this comment

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

The step is just:

  1. File an issue.
  2. Get LGTM from 2 core committers.

Copy link
Copy Markdown
Member Author

@mrjoro mrjoro Feb 22, 2017

Choose a reason for hiding this comment

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

Changed the link to specifically point to the core committers section. I also changed TL to OWNER since I think that's the intent now?

Was your suggestion to move most of the bullet points you added in GOVERNANCE to here?

CONTRIBUTING.md Outdated

When you create a Pull Request a check will be run to ensure that you have signed the CLA. Make sure that you sign the CLA with the same email address you associate with your commits (likely via the `user.email` Git config as described on GitHub's [Set up Git](https://help.github.com/articles/set-up-git/) page).

## Ongoing Participation
Copy link
Copy Markdown
Member

@cramforce cramforce Feb 20, 2017

Choose a reason for hiding this comment

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

Sentence case :)

Copy link
Copy Markdown
Member Author

@mrjoro mrjoro Feb 20, 2017

Choose a reason for hiding this comment

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

Done

@mrjoro mrjoro mentioned this pull request Feb 20, 2017
Closed
cramforce
cramforce approved these changes Feb 22, 2017
View reviewed changes
CONTRIBUTING.md
The AMP Project is meant to evolve with feedback. The project and its users appreciate your thoughts on ways to improve the design or features.

**Bugs**
To make a suggestion [file an issue](https://github.com/ampproject/amphtml/issues/new) and add the [Type: Feature Request](https://github.com/ampproject/amphtml/labels/Type%3A%20Feature%20Request) label.
Copy link
Copy Markdown
Member

@cramforce cramforce Feb 22, 2017

Choose a reason for hiding this comment

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

Yes, good call.

@mrjoro
Copy link
Copy Markdown
Member Author

mrjoro commented Feb 22, 2017

I'm not authorized to merge; merge plz?

@cramforce cramforce merged commit 997e46a into ampproject:master Feb 22, 2017
@mrjoro mrjoro deleted the clean-developing branch February 22, 2017 16:45
mrjoro added a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
@mrjoro
Reconcile/clarify contribution & development docs (ampproject#7662)
d09bb81
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cramforce cramforce cramforce approved these changes

Assignees

@mrjoro mrjoro

Labels

Related to: Documentation

Projects

None yet

Milestone

Docs Updates

Development

Successfully merging this pull request may close these issues.

2 participants

@mrjoro @cramforce