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

documentation #3044

Merged
merged 19 commits into from
Jul 7, 2018
Merged

documentation #3044

merged 19 commits into from
Jul 7, 2018

Conversation

vijithassar
Copy link
Contributor

assorted changes to the first half or so of developer_docs/README.md

@vijithassar vijithassar requested a review from lmccart June 28, 2018 08:37
@vijithassar vijithassar force-pushed the documentation branch 2 times, most recently from 4c33025 to 8abd6d8 Compare June 30, 2018 20:59
Copy link
Member

@lmccart lmccart left a comment

Choose a reason for hiding this comment

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

it's great. the one change requested is to separate out the example info info reference examples vs examples page. thanks @vijithassar!


* **Contribute in some other way** :
The developer tooling included with the p5.js codebase is intentionally very strict about some things. This is a good thing! It makes everything consistent, and it will encourage you to be disciplined. This means you may try to change something only to have your commit rejected by the project, but don't get discouraged; even seasoned p5.js developers get caught by this stuff all the time. Typically the problem will be in one of two areas.
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

@vijithassar vijithassar Jul 7, 2018

Choose a reason for hiding this comment

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


Add to the examples on the [p5js.org examples page](http://p5js.org/examples/). See this [guide](https://github.com/processing/p5.js-website/wiki/Adding-examples) for details.
The p5.js reference manual includes [integrated examples](http://p5js.org/examples/). You can [add more](https://github.com/processing/p5.js-website/wiki/Adding-examples), and there is an issue which lists some [desired examples](https://github.com/processing/p5.js/issues/1954).
Copy link
Member

Choose a reason for hiding this comment

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

this is actually the link for the examples page p5js.org/examples
the reference example instructions are here: https://github.com/processing/p5.js/blob/master/developer_docs/inline_documentation.md


* **Add examples to the examples page** :
The p5.js reference manual includes [integrated examples](http://p5js.org/examples/). You can [add more](https://github.com/processing/p5.js-website/wiki/Adding-examples), and there is an issue which lists some [desired examples](https://github.com/processing/p5.js/issues/1954).
Copy link
Member

Choose a reason for hiding this comment

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

so there are two different examples places:

  1. p5js.org/reference -- instructions here and desired examples
  2. p5js.org/examples -- instructions here. these live in the p5.js-website repo which is why the instructions are over there. however, I would like this possible contribution option to be mentioned in this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't this need covered by "ways to contribute > accompaniments > inline documentation" or am I misunderstanding you?

Copy link
Member

Choose a reason for hiding this comment

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

oh I see, I thought the first link pointed to the reference page because I saw the phrase "p5.js reference". so I think the change here is just to write "p5.js website includes" instead of "p5.js reference manual includes", since I would assume reference manual refers to p5.js/reference which is actually a different set of examples. does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


## Unit Tests

Unit tests are small pieces of code which are created as complements to the primary logic and perform validation. If you are developing a major new feature for p5.js, you should probably include tests. Do not submit pull requests in which the tests do not pass, because that means something is broken.
Copy link
Member

Choose a reason for hiding this comment

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

"you should probably include tests" > "you will need to include tests"
we will likely be unable to merge new feature pull requests without unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I phrased it this way because "new feature" is intentionally broad and could include all sorts of things, not just a new method that can be unit tested or whatever. I also figured we didn't want this documentation to scare off less experienced users who might not yet know what unit tests are, and can coach them through that if they send in a pull request that needs them to be added.

Copy link
Member

Choose a reason for hiding this comment

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

👌i agree

README.md functions as an index, contains information itself, and covers a variety of topics, so it's probably better to be less specific when briefly explaining what it is trying to accomplish
explanation of GitHub Issues was under a section heading about bug fixes, which is not accurate because Issues also apply to features
combine JSDoc comments, unit tests, and benchmarks under one section heading, because they are all best practices that may sometimes be optional depending on the nature of the changes being made to the code
simplify language and remove redundancies
edit the description of the workflow
group the various ways to help together in a single section
add changes and commits to the description of the development workflow
include prompt to specify shell commands and explicitly mention pushing changes back to the remote fork
explain all the reasons why a commit might be rejected in one place
remove extra backticks
topics like branching, pull requests, and avoiding merge conflicts are probably beyond the scope of this document
explain that the $ prompt should not be typed before the first shell command
explain in-browser testing as part of the existing unit testing section
@vijithassar
Copy link
Contributor Author

@lmccart lmccart merged commit dab5ab3 into processing:master Jul 7, 2018
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.

2 participants