From 8d1f33860bd48f0dfde3ed8aa9c7052614bf0123 Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Tue, 10 Nov 2015 13:55:06 -0500 Subject: [PATCH 1/4] Spell check != grammar check --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e4864595..354bc999 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -19,7 +19,7 @@ Use the [issue tracker](https://github.com/lhorie/mithril.js/issues). Please do We welcome any and all contributions. This is a community-driven project. Although we don't have a lot, we do have a few guidelines for contributions. -1. Please adhere to the style guide. It's pretty lengthy, but most everything is checked by ESLint, and if anything fails to check, the build will fail, and you will know about it. Most of it is common practice and common sense. ESLint is also set up to check for other common errors, such as undeclared variables and invalid `typeof` values. +1. Please adhere to the style guide. It's pretty lengthy, but most of it is checked by ESLint, and if anything fails to check, the build will fail, and you will know about it. Most of it is common practice and common sense. ESLint is also set up to check for other common errors, such as undeclared variables and invalid `typeof` values. 2. Please make sure there are no regressions with your patch. Please don't disable existing tests, and please don't send a PR with new, disabled tests. - There are a few known failing tests currently (PRs welcome): [*](https://github.com/lhorie/mithril.js/blob/next/test/mithril.deferred.js#L121-L125) [*](https://github.com/lhorie/mithril.js/blob/next/test/mithril.render.js#L1321-L1343) [*](https://github.com/lhorie/mithril.js/blob/next/test/mithril.route.js#L106-L131) [*](https://github.com/lhorie/mithril.js/blob/next/test/mithril.route.js#L134-L162) [*](https://github.com/lhorie/mithril.js/blob/next/test/mithril.route.js#L165-L191) [*](https://github.com/lhorie/mithril.js/blob/next/test/mithril.route.js#L194-L222) 3. For any new features introduced, be sure to write new unit tests for it. Maximum coverage is what we want. From d4b618d2afb213884aa71fa65153a343b156a25d Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Tue, 10 Nov 2015 14:24:31 -0500 Subject: [PATCH 2/4] Clarify a few things [ci skip] --- CONTRIBUTING.md | 51 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 354bc999..b492bb37 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -25,12 +25,13 @@ We welcome any and all contributions. This is a community-driven project. Althou 3. For any new features introduced, be sure to write new unit tests for it. Maximum coverage is what we want. 4. Try to not leave any extra `TODO`s, `FIXME`s, etc. in your code. ESLint will nag at you until you fix whatever problem it is. - Note that it's only a warning, not an error. It won't fail the CI tests, and there's a few outstanding ones inside Mithril right now. + - If you must, use a `TODO(): whatever` (or the equivalent `FIXME`, etc.) if it's something you are actively working on, or if it's something more general, file an issue titled "TODO: " and reference the `TODO` comment itself. -It is assumed that all contributions you make you have the appropriate rights to and that it may be made available under the MIT License, the license used for this project. +It is assumed that you have the appropriate rights to all contributions that you make, and that they can be made available under the MIT License which is used for this project. # Style Guide -The style is checked with ESLint. This style guide is here for one reason only: consistency. This should work for most code here, but this shouldn't be considered absolute &endash; consistency with the surrounding code is higher priority than following this guide. Also, if some sort of hack is impossible without violating this guide (e.g. if Mithril ever needs [Bluebird's infamous hack](https://stackoverflow.com/q/24987896)), just make sure the code is consistent with what's around it. +The style is checked with ESLint. This style guide is here for one reason only: consistency. This should work for most code here, but this shouldn't be considered absolute &endash; consistency with the surrounding code is higher priority than following this guide. Also, if some sort of hack is impossible without violating this guide (e.g. if Mithril ever needs [Bluebird's infamous hack](https://stackoverflow.com/q/24987896)), just make sure the code is consistent with what's around it, and that you document how the hack works if you can, and why the hack was employed. If it's a more common one such as Bluebird's, a direct link to a description suffices for how. ### Line length @@ -245,7 +246,11 @@ function iterate() { ### Trailing whitespace -Please don't leave trailing spaces anywhere. It makes diffs harder to read. +Please don't leave trailing whitespace anywhere. It makes diffs harder to read. + +Blank lines should have no indentation or spaces. It should be empty other than a line ending. + +All files should end with a trailing space. Some editors add this on their own, so it will result in unnecessary lines in later diffs if you don't. ### Indentation and vertical whitespace @@ -793,6 +798,46 @@ if (condition) { } else doSomethingElse() ``` +### `do`-`while` loops + +This should be avoided in most cases, since the only good use case is if the intent is "do this, and then check" each iteration. This should only be used in cases like below: + +```js +// Form 1 +doSomething() + +while (condition) { + doSomething() +} + +// Form 2 +while (true) { + doSomething() + if (!condition) break +} +``` + +Always use braces. Also, there must be a semicolon at the end of the `while` condition. This is the only exception to the semicolon rule, since the omission is a syntax error. The semicolon is never automatically inserted before ES6. + +```js +// Good +do { + doSomething() +} while (condition); + +// Bad +do doSomething() +while (condition); + +// Syntax errors +do { + doSomething() +} while (condition) + +do doSomething() +while (condition) +``` + ### Empty blocks Mark empty blocks as intentionally empty via a comment or similar. Don't just leave an empty block and/or semicolon. From 7eb2c9a5b1581733bcdde3387e60edef74b96dd2 Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Tue, 10 Nov 2015 14:26:12 -0500 Subject: [PATCH 3/4] Add mention of browser [ci skip] --- CONTRIBUTING.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b492bb37..0d33390d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,8 +3,9 @@ Use the [issue tracker](https://github.com/lhorie/mithril.js/issues). Do check to make sure your bug hasn't already been filed. Please give the following information, where possible: 1. The version of Mithril you're using, whether it's the dev version or [the version on npm](http://npm.im/mithril.js). The version on npm may not have all the latest bug fixes, so your bug might very well be fixed in the dev version. -2. A detailed explanation of the bug. -3. A test case. The simpler, the better. +2. The name and version of the browser(s) affected. +3. A detailed explanation of the bug. +4. A test case. The simpler, the better. # Feature requests From 2ccd07b8f0274cb5dc35fc88a41ac3bff86428ac Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Tue, 10 Nov 2015 14:43:10 -0500 Subject: [PATCH 4/4] Remove redundant part, s/space/line break/ --- CONTRIBUTING.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0d33390d..9c17170a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,8 +52,6 @@ This isn't checked for the tests, but still, keep it reasonable. Use Windows-style line endings (i.e. CRLF). -End each file with a line break. - ### Semicolons Avoid semicolons. (A few resources to help you understand this: [*](http://blog.izs.me/post/2353458699/an-open-letter-to-javascript-leaders-regarding) [*](http://inimino.org/~inimino/blog/javascript_semicolons) [*](http://jamesallardice.com/understanding-automatic-semi-colon-insertion-in-javascript/)) @@ -251,7 +249,7 @@ Please don't leave trailing whitespace anywhere. It makes diffs harder to read. Blank lines should have no indentation or spaces. It should be empty other than a line ending. -All files should end with a trailing space. Some editors add this on their own, so it will result in unnecessary lines in later diffs if you don't. +All files should end with a trailing line break. Some editors add this on their own, so it will result in unnecessary lines in later diffs if you don't. ### Indentation and vertical whitespace