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 contributing docs #1574

Merged
merged 5 commits into from
Oct 9, 2017
Merged

Update contributing docs #1574

merged 5 commits into from
Oct 9, 2017

Conversation

EnTeQuAk
Copy link
Contributor

This got heavily inspired by the web-ext contribution docs, kudos for
them!

Fix #1572

  • remove grunt commands that are already documented via 'scripts' section

This got heavily inspired by the web-ext contribution docs, kudos for
them!

Fix #1572
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e2655a5 on 1572-update-contribution-docs into d3e429f on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e2655a5 on 1572-update-contribution-docs into d3e429f on master.

CONTRIBUTING.md Outdated
process of contributing patches, have a read through these
[good first bugs](https://github.com/mozilla/web-ext/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+bug%22).

If you are already familiar with the process or want to contribute even more we appreciate any help on our [contrib: welcome](https://github.com/mozilla/addons-linter/issues?q=is%3Aissue+is%3Aopen+label%3A"contrib%3A+welcome) issues.
Copy link
Contributor

@muffinresearch muffinresearch Sep 28, 2017

Choose a reason for hiding this comment

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

Maybe something like this:

If you're already familiar with the project or would like take on something 
a little more challenging, please take a look at the 
[contrib: welcome](%LINK%) issues.

Copy link
Contributor

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I have some editorial comments but if you make them I'd say r+wc 😄

@@ -1,20 +1,144 @@
Thanks for wanting to contribute to Mozilla's Add-ons Linter! You rock! 😊
Hi! Thanks for wanting to contribute to Mozilla's Add-ons Linter! You rock! 😊
Copy link
Contributor

Choose a reason for hiding this comment

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

tenor

CONTRIBUTING.md Outdated

## Submitting a Pull Request
The linter is used in various stages of [WebExtension](https://developer.mozilla.org/en-US/Add-ons/WebExtensions)
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is phrased a bit awkwardly and it makes it hard to parse. Also I'm pretty sure we're not supposed to say WebExtensions? I dunno. Saying "contributions are awesome" is a bit of a generic call-to-action as well, but I'd encourage people involved with extensions to relay their experience and get involved... I'd try:

This linter is used to help develop and publish [extensions](https://developer.mozilla.org/en-US/Add-ons/WebExtensions) for Firefox. You're an add-on developer, we would really value your contributions–no one knows add-on development and publishing better than an add-on developer!

CONTRIBUTING.md Outdated
Please run the tests locally with `npm test` before you commit.
- [Picking an issue](#picking-an-issue)
- [Installation](#installation)
- [Develop all the things](#develop-all-the-things)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that "All the things" is a cute meme but not everyone gets memes, especially non-Western, English-speaking folks. This could be better written as "Development environment".

But actually looking at that section I think it can be removed and folded into testing.

CONTRIBUTING.md Outdated
- [Picking an issue](#picking-an-issue)
- [Installation](#installation)
- [Develop all the things](#develop-all-the-things)
- [Run all application tests](#run-all-application-tests)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be "Testing the linter" and the "develop all the things" section should be moved under it to "Run tests when you make changes"

CONTRIBUTING.md Outdated

# Picking an issue

If you're looking for a small task to work on so you can get familiar with the
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change this whole paragraph to:

For first-time contributors or those who want to start with a small task: [check out our list of good first bugs](https://github.com/mozilla/web-ext/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+bug%22). These issues have an assigned mentor to help you out and are great issues for learning about the linter and our development process.

CONTRIBUTING.md Outdated

npm test

The other commands below are just variations on this.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this line.

CONTRIBUTING.md Outdated
Fixes #123

The issue number in this case is "123."
The word *Fixes* is magical; github will automatically close the issue when your
Copy link
Contributor

Choose a reason for hiding this comment

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

"github" -> "GitHub"

CONTRIBUTING.md Outdated

Good commit messages serve at least three important purposes:

* To speed up the reviewing process.
Copy link
Contributor

Choose a reason for hiding this comment

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

The "to" in front of these bullet points aren't quite right... I'd say "they" instead of "to".

CONTRIBUTING.md Outdated

* To speed up the reviewing process.
* To help us write a good release note.
* To help the future maintainers of addons-linter (it could be you!), say five years into the future, to find out why a particular change was made to the code or why a specific feature was added.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is too long especially compared to the others. I'd just say "They help future maintainers understand your change and the reasons behind it."

CONTRIBUTING.md Outdated
Good commit messages serve at least three important purposes:

* To speed up the reviewing process.
* To help us write a good release note.
Copy link
Contributor

Choose a reason for hiding this comment

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

"They help us write good release notes"

@EnTeQuAk EnTeQuAk force-pushed the 1572-update-contribution-docs branch from 12ddf20 to bd3ff7b Compare October 6, 2017 17:46
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e3f00a6 on 1572-update-contribution-docs into 8fc2c5c on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e3f00a6 on 1572-update-contribution-docs into 8fc2c5c on master.

CONTRIBUTING.md Outdated

## Submitting a Pull Request
This linter is used to help develop and publish [Add-ons](https://developer.mozilla.org/en-US/Add-ons/) for Firefox. You're an add-on developer, we would really value your contributions–no one knows add-on development and publishing better than an add-on developer!
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to remove the /en-US/ directory and let MDN redirect to the user's preferred locale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good point, damn copy & paste. Thanks!

@EnTeQuAk EnTeQuAk merged commit 08e2b1e into master Oct 9, 2017
@EnTeQuAk EnTeQuAk deleted the 1572-update-contribution-docs branch October 9, 2017 07:50
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3f7c14d on 1572-update-contribution-docs into 8fc2c5c on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3f7c14d on 1572-update-contribution-docs into 8fc2c5c on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants