docs(CONTRIBUTING): centralize, update, improve #16297

Narretz · 2017-10-24T17:11:53Z

remove outdated info
move info about writing docs from Wiki to contribute.md + updates
move info about development setup from docs/contribute to contribute.md
add info about running tests on Saucelabs / Browserstack
add info about specific core code topics
add link to contributing.md to Contribung page
add link to changelog.md to nav in docs

Closes #7303
Closes #9444

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

- remove outdated info - add info about writing docs from Wiki - add info about development setup from docs contribute.md - add info about running tests on Saucelabs / Browserstack TODO: - add link to contributing.md to Contribung page - add link to changelog.md to dropdown in docs -- link to current version changelog entry? Closes angular#7303 Closes angular#9444

gkalpak

💯 00 - Love the overhaul.

But I do think that all the docs related bits make CONTRIBUTING.md quite overwhelming and intimidating. I would still keep it as a separate doc and link to it from CONTRIBUTING.md. WDYT?

gkalpak · 2017-10-25T10:07:14Z

docs/content/misc/contribute.ngdoc


+For everything related to contributing, we have a document in our Git Repository that covers everything from submitting issues, setting up and building the code, to writing documentation, and much more: [Contributing](https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md).


Finally, let me say this:

Super long lines in markdown files are evil, hard to review and hard to work with without special settings in your local editor. So why on earth would you write such lines in the first place when you can easily split them at 100 characters and get a nice readable, reviewable block of text? I hope this very long line forced you to scroll horizontally and helped you realize how unnecessarily unpleasant long lines can be. If you made it this far, thank you for allowing me to waste a small part of your time and for taking this comment into consideration.

_{(Yes, long lines can be fun, but only if you are me 😁)}

I didn't have to scroll because I read it via email. 😛
But I will cut the lines. I did write this in an editor with word wrap ...

Arghhh... Well, it was worth a try 😛

gkalpak · 2017-10-25T10:10:14Z

CONTRIBUTING.md



 [Google Closure I18N library]: https://github.com/google/closure-library/tree/master/closure/goog/i18n
 [angular-dev]: https://groups.google.com/forum/?fromgroups#!forum/angular-dev
+[browserstack]: https://www.browserstack.com/
 [closing-issues]: https://help.github.com/articles/closing-issues-via-commit-messages/
 [coc]: https://github.com/angular/code-of-conduct/blob/master/CODE_OF_CONDUCT.md
 [commit-message-format]: https://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y/edit#
 [contribute]: http://docs.angularjs.org/misc/contribute
 [contributing]: http://docs.angularjs.org/misc/contribute


This can go now 😃

gkalpak · 2017-10-25T20:56:24Z

CONTRIBUTING.md


 ## <a name="coc"></a> Code of Conduct

 Help us keep AngularJS open and inclusive. Please read and follow our [Code of Conduct][coc].

+## <a name="requests"></a> Questions, Bugs, Features
+
 ## <a name="question"></a> Got a Question or Problem?


This and following sections should be ###.

gkalpak · 2017-10-25T20:58:24Z

CONTRIBUTING.md

-If you have questions about how to use AngularJS, please direct these to the [Google Group][groups]
-discussion list or [StackOverflow][stackoverflow]. We are also available on [IRC][irc] and
-[Gitter][gitter].
+Do not open issues for general support questions as we want to keep GitHub issues for bug reports and feature requests. You've got much better chances of getting your question answered on dedicated support platforms, the best being [Stack Overflow][stackoverflow]:


[stackoverflow]: --> [stackoverflow].

gkalpak · 2017-10-25T21:02:25Z

CONTRIBUTING.md

+* **Major Changes** that you wish to contribute to the project should be discussed first in an
+  [GitHub issue][github-issues] that clearly outlines the changes and benefits of the feature.
+* **Small Changes** can directly be crafted and submitted to the [GitHub Repository][github]
+  as a Pull Request. See the main section about [Documentation & Code Contributions](#code-contrib)


code-contrib --> contributions

gkalpak · 2017-10-25T22:18:44Z

CONTRIBUTING.md

+    ```
+
+* In GitHub, send a pull request to `angular.js:master`. This will trigger the check of the
+(Contributor License Agreement)(#cla) and the Travis integration.


(Contributor License Agreement) --> [Contributor License Agreement]

gkalpak · 2017-10-25T22:20:08Z

CONTRIBUTING.md

+### Writing runnable (live) examples and e2e tests
+It is possible to embed examples in the documentation along with appropriate e2e tests. These
+examples and scenarios will be converted to runnable code within the documentation.  So it is important
+that they work correctly.  To ensure this, all these e2e scenarios are run as part the automated Travis tests.


as part the automated --> as part of the/our automated

gkalpak · 2017-10-25T22:22:27Z

CONTRIBUTING.md

+that they work correctly.  To ensure this, all these e2e scenarios are run as part the automated Travis tests.
+
+If you are adding an example with an e2e test, you should [run the test locally](#e2e-tests). You can
+use `fit` to run only your test.


I would expand a little bit on that, e.g.:

...you should run the test locally first to ensure it passes.
You can change it(...) to fit(...) to run only your test, but make sure you change it back to it(...) before committing.

gkalpak · 2017-10-25T22:23:19Z

CONTRIBUTING.md

+This tag identifies a block of HTML that will define a runnable example. It can take the following attributes:
+
+* `module` - specify an AngularJS module containing code that must be loaded to support this example.
+* `animation` - if set to `true` then this example uses ngAnimations.


ngAnimations --> `ngAnimate`

gkalpak · 2017-10-25T22:24:15Z

CONTRIBUTING.md

+
+* `module` - specify an AngularJS module containing code that must be loaded to support this example.
+* `animation` - if set to `true` then this example uses ngAnimations.
+


What about the name and deps attributes?

Narretz

Thanks for the thorough review, @gkalpak !
I'll have to look into automatically checking the links. So many are wrong.

I don't know if separating "Writing docs" makes a lot of sense, as it's an important part of writing a PR. We could move it at the end of Code & Doc Contrib.

I've also seen some repos that have a special DEVELOPERS.md, which contains everything from project setup to coding rules.

Narretz · 2017-10-26T08:49:24Z

CONTRIBUTING.md

+
+#### Throwing errors
+
+User-facing errors should be thrown with [`minErr`][minErr], a special error function that provides errors ids, templated error messages, and adds a link to a detailed error description.


We also have many bollean provider config functions that don't end in enabled :D

Narretz · 2017-10-26T08:49:55Z

CONTRIBUTING.md

+* `@requires` - normally indicates that a JavaScript module is required; in an Angular service it is used to describe what other services this service relies on
+* `@property` - describes a property of an object
+* `@description` - used to provide a description of a component in markdown
+* `@link` - specifies a link to a URL or a type in the API reference. **NOTE**: to link to `ng.$rootScope.Scope#$on` insert `methods_` between `#` and the actual method name: `{@link ng.$rootScope.Scope#methods_$on listen}`. Same goes for properties and events.


I actually didn't know about it either, this is copy and paste from the Wiki

Narretz · 2017-10-26T08:52:39Z

docs/content/misc/contribute.ngdoc


+For everything related to contributing, we have a document in our Git Repository that covers everything from submitting issues, setting up and building the code, to writing documentation, and much more: [Contributing](https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md).


I didn't have to scroll because I read it via email. 😛
But I will cut the lines. I did write this in an editor with word wrap ...

gkalpak · 2017-10-30T10:52:30Z

Here is my reasoning:

Since CONTRIBUTING.md is the first resource for potential contributors (including first-time contributors), I think it should be as lean as possible containing what is likely to be useful to many people (e.g. CoC, commit message guidelines, etc) and link to other, more specific resources. For example, many first-time contributors will just fix a typo of improve the wording in a description. Most of the time, they don't need to know everything about are (awesome but non-trivial) documentation pipeline and having a huge CONTRIBUTING.md may (a) make it more difficult to find what they really need to know and (b) be intimidating (for newcomers) 😃

I don't necessarily suggest moving everything that could be irrelevant to first-time contributors to other files, but the docs authoring section seemed/felt substantial in size and quite dense. Maybe moving dev related things to DEVELOPERS.md is a good compromize. (I would still keep the Commit Message Guidelines section in CONTRIBUTING.md though 😁)

_{I don't feel too strongly about it, so happy to go either way.}

frederikprijck · 2017-10-30T11:43:55Z

CONTRIBUTING.md

+  globally with:
+
+  ```shell
+  yarn global add grunt-cli


Wouldn't it be a better idea to avoid global installation of grunt by default ?
People unfamiliar with yarn/npm will follow the guideline and install it globally. I think people should not install this globally unless they know why.

Sorry, I'm a bit sensitive on the global npm package installations 🙈 It's an "anti-pattern" used in to many projects.

As discussed in #16276, this is something we can do: document yarn ... as the default and list global installing of grunt as an alternative. But I think this can be part of a follow-up PR, as there may be more files that are affected by this.

petebacondarwin · 2017-10-30T11:57:01Z

I agree with @gkalpak that we should move some of the more technical stuff to DEVELOPERS.md.

Narretz · 2017-10-30T12:04:34Z

Ok. I think putting these 3 into the DEVELOPERS.md is a good separation, with links from CONTRIBUTING.md where it's appropriate:

Development Setup
Coding Rules
Writing Documentation

Narretz · 2017-11-02T10:47:21Z

I've created the new DEVELOPERS.md, which contains the following sections:

Development Setup
Coding Rules
Commit Message Guidelines
Writing Documentation

I've added the commit message guidelines, because ...

they are quite dense, and we want to have a low entry barrier for contributors
for "improve docs" commits the message is pre-formatted (and easy to amend for us)
we now check the commit message on PRs in Travis

petebacondarwin · 2017-11-02T13:25:08Z

DEVELOPERS.md

+
+  ```shell
+  yarn global add grunt-cli
+  ```


Did we not decide to avoid installing grunt globally?
Instead people can just use yarn grunt ... to run grunt tasks.

petebacondarwin · 2017-11-02T13:25:49Z

DEVELOPERS.md

+
+```shell
+sudo yarn global add grunt-cli
+sudo yarn global add bower


I think we have ditched bower

We still need it for a few things. @mgol will switch us to yarn aliases some time in the future.

petebacondarwin · 2017-11-02T13:26:18Z

DEVELOPERS.md

+`grunt` and `bower`:
+
+```shell
+sudo yarn global add grunt-cli


If we use local grunt then this whole note can be removed.

petebacondarwin · 2017-11-02T13:27:16Z

DEVELOPERS.md

+
+* `docs/` — A directory that contains a standalone version of the docs
+  (same as served in `docs.angularjs.org`).
+


Perhaps we ought to mention it also contains all the other core module files too? (e.g. angular-mocks.js, angular-route.js etc).

petebacondarwin

I think we should use local grunt and drop the global installs of grunt and bower.

Otherwise LGTM

Narretz · 2017-11-02T15:59:44Z

I think we can add the local grunt changes in a follow-up commit (and also note that you can still install globally grunt if you want ... that saves keystrokes!)

mgol · 2017-11-02T16:41:16Z

We should avoid global packages but since the relevant doc was just ported from an existing one, I'm fine doing that in a followup PR.

gkalpak

Can you check the links in CONTRIBUTING.md and DEVELOPERS.md and ensure they are valid before merging?

gkalpak · 2017-11-02T17:04:55Z

CONTRIBUTING.md

-  [commit message conventions](#commit) and passes our commit message presubmit hook
-  (`validate-commit-msg.js`). Adherence to the [commit message conventions](#commit) is required,
-  because release notes are automatically generated from these messages.
+  [commit message conventions][developers.commits]. Adherence to the [commit message conventions](#commit)


#commit is no longer a valid link.

gkalpak · 2017-11-02T17:06:13Z

CONTRIBUTING.md

@@ -129,27 +146,33 @@ Before you submit your pull request consider the following guidelines:
 * Push your branch to GitHub:

    ```shell
-    git push origin my-fix-branch
+    git push <remote> my-fix-branch


Why not origin. I'm afraid <remote> will confuse git beginners (and the instructions do not matter for experiences users, because they already know their way around).

Right, origin is better. I was thinking of my setup where angular/angular.js is the origin, and my fork just a remote.
Technically, since git now uses "simple" as the default push strategy, should git push be enough?

I think that for branches created in the way we show here, you need to have set the upstream once before you can just git push. I.e. you need to have run git push --set-upstream origin my-fix-branch the first time.

CONTRIBUTING.md - focus on basic info about issues and pull requests for new contributors - move development heavy info to DEVELOPERS.md + add links - remove outdated info DEVELOPERS.md - contains info about project setup, coding rules, and commit message guidelines from CONTRIBUTING.md - add and update info about writing docs from Wiki - add info about development setup from docs contribute.md - add info about running tests on Saucelabs / Browserstack Closes #7303 Closes #9444 Closes #16297

Narretz added type: chore type: docs labels Oct 24, 2017

googlebot added the cla: yes label Oct 24, 2017

Narretz force-pushed the docs-contributing branch from 00917d7 to e5bbe9c Compare October 24, 2017 17:14

Narretz force-pushed the docs-contributing branch from e5bbe9c to fee98e6 Compare October 25, 2017 09:04

gkalpak suggested changes Oct 25, 2017

View reviewed changes

Narretz commented Oct 26, 2017

View reviewed changes

fixup!

a81da61

Narretz force-pushed the docs-contributing branch from 07d9abb to a81da61 Compare October 27, 2017 10:55

frederikprijck reviewed Oct 30, 2017

View reviewed changes

Narretz force-pushed the docs-contributing branch 8 times, most recently from a006250 to 8f7ef70 Compare November 2, 2017 10:43

Narretz force-pushed the docs-contributing branch 2 times, most recently from 082b8d8 to d3d257d Compare November 2, 2017 11:06

petebacondarwin reviewed Nov 2, 2017

View reviewed changes

petebacondarwin suggested changes Nov 2, 2017

View reviewed changes

gkalpak approved these changes Nov 2, 2017

View reviewed changes

Narretz force-pushed the docs-contributing branch 2 times, most recently from 4da38bc to 01c31b3 Compare November 3, 2017 11:20

fixup! create DEVELOPERS.md

a027fa5

Narretz force-pushed the docs-contributing branch from 01c31b3 to a027fa5 Compare November 3, 2017 11:35

Narretz merged commit 181ac0b into angular:master Nov 3, 2017


		For everything related to contributing, we have a document in our Git Repository that covers everything from submitting issues, setting up and building the code, to writing documentation, and much more: [Contributing](https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md).


		* `module` - specify an AngularJS module containing code that must be loaded to support this example.
		* `animation` - if set to `true` then this example uses ngAnimations.


		#### Throwing errors

		User-facing errors should be thrown with [`minErr`][minErr], a special error function that provides errors ids, templated error messages, and adds a link to a detailed error description.


		* `docs/` — A directory that contains a standalone version of the docs
		(same as served in `docs.angularjs.org`).

docs(CONTRIBUTING): centralize, update, improve #16297

docs(CONTRIBUTING): centralize, update, improve #16297

Conversation

Narretz commented Oct 24, 2017 • edited Loading

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Oct 30, 2017

frederikprijck Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 30, 2017

Narretz commented Oct 30, 2017

Narretz commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Narretz commented Nov 2, 2017

mgol commented Nov 2, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Oct 24, 2017 •

edited

Loading

frederikprijck Oct 30, 2017 •

edited

Loading

Narretz Nov 3, 2017 •

edited

Loading