Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

chore: code style change, lint using modified eslint-config-airbnb #134

Merged
merged 6 commits into from Jul 6, 2015

Conversation

vvo
Copy link
Contributor

@vvo vvo commented Jul 5, 2015

Following #128 and various discussions we already had on code style, I
configured our lint on airbnb/javascript style guide using eslint.

I chose to stay with ESLint because:

  • it's becoming a good linting standard and has the best support for both ES6,
    jsx, react..
  • already what we know, do not have to change again
  • other alternatives were not as good as what we wanted

Not a single linter has a "Automatically reformat this whole code
option, do it well". ESLint has some wishes for ESLint 2.0 but may be
later.

In the end we will struggle a little bit on following our own Algolia
style at the begining but it will be worth it soon as we will be more
and more JavaScript`er. I recall @bobylito wanting to do so early on
when we both joined. Well, now it's time!

The goal of this PR is to provide a solid ground for our linting rules
amongst all our JS projects.

The currently picked coding style is the one from Airbnb, with
modifications.

You can find the modified rules here: https://github.com/algolia/eslint-config-algolia#rules

One of the not so obvious rule is the semi colon remove, I was convinced
by https://github.com/feross/standard#rules and
https://github.com/yyx990803/semi#but-semicolons-are-required

I assume there are some rules like spacing aroud parentheses and
brackets that will be a big change for you @bobylito (and it's your
project so I am dropping your name here!)

But we need to try to find a style that is both OUR style and also will
not give our future open source contributors too much WTF moments when
encountering our style.

On this subject, http://sideeffect.kr/popularconvention/#javascript is a
good data based source.

We can discuss, add, remove, edit the rules as long as we do not discuss them
every week!

Other projects:

  • Original feross/standard:
    After trying it I discovered some rules were not strict enough and
    would lead to again style discussions. Has some sort of formatter but
    not very good. Based on eslint.

Ref:

Ultimately eslint will have a good formatter, in the meantime we will
all switch to some new coding style, for good :-)?

fixes #128

@vvo vvo mentioned this pull request Jul 5, 2015
@vvo vvo force-pushed the chore/use-eslint-config-algolia branch from 1d10458 to d5d9f24 Compare July 5, 2015 21:44
@vvo
Copy link
Contributor Author

vvo commented Jul 5, 2015

@@ -62,19 +62,19 @@ helper.search();

[See the examples in action](http://algolia.github.io/algoliasearch-helper-js/)

### Use with NPM
### With NPM
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a side note, maybe that's not the right PR for that ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will revert this

Copy link
Contributor

Choose a reason for hiding this comment

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

But that's not a big deal ;)

@bobylito
Copy link
Contributor

bobylito commented Jul 6, 2015

Imho your PR shouldn't contain the fixes for the CI...

@vvo
Copy link
Contributor Author

vvo commented Jul 6, 2015

Imho your PR shouldn't contain the fixes for the CI...

Agree. I only added them here in thinking that if we merge this then I would not have to re apply the rules to the test files, I will revert the commit and submit a new PR targeting this current PR

@bobylito
Copy link
Contributor

bobylito commented Jul 6, 2015

Ok so I went through the whole PR. Personally speaking I don't feel it actually matches "my style" visually :). The points that actually made me thinking were :

  • the semicolon
  • the else / else if statements on the same line that the previous if / else if block end
  • the single quote

But really I shouldn't be cry baby here, that's awesome to get a common style :) 👍

@vvo
Copy link
Contributor Author

vvo commented Jul 6, 2015

On the semicolon side that's also a big change for me but I do have the feeling that in one month if we apply it everywhere we code we will love it!

We only need to make sure we do add the semicolon when we document things like on the website. On adding/removing semicolon easily on a whole project: https://www.npmjs.com/package/semi

I know it's an even bigger style change for you (more changes) and I appreciate you still find it awesome that we aim at a common style!

Gonna make some changes we talked and will ping again here

@pixelastic
Copy link
Contributor

Yep, no semi-colon is a big change for me, but still in favor of any unified codestyle. There is also a package to convert double quotes to single quotes if you ever need it.

@vvo vvo force-pushed the chore/use-eslint-config-algolia branch from 30d574e to 1a2a332 Compare July 6, 2015 08:06
Following #128 and various discussions we already had on code style, I
configured our lint on airbnb/javascript style guide using eslint.

I chose to stay with ESLint because:
- it's becoming a good linting standard and has the best support for
both ES6,
jsx, react..
- already what we know, do not have to change again
- other alternatives were not as good as what we wanted

Not a single linter has a "Automatically reformat this whole code
option, do it well". ESLint has some wishes for ESLint 2.0 but may be
later.

In the end we will struggle a little bit on following our own Algolia
style at the begining but it will be worth it soon as we will be more
and more JavaScript`er. I recall @bobylito wanting to do so early on
when we both joined. Well, now it's time!

The goal of this PR is to provide a solid ground for our linting rules
amongst all our JS projects.

The currently picked coding style is the one from Airbnb, with
modifications.

You can find the modified rules here: https://github.com/algolia
/eslint-config-algolia#rules

I assume there are some rules like spacing aroud parentheses and
brackets that will be a big change for you @bobylito (and it's your
project so I am dropping your name here!)

But we need to try to find a style that is both OUR style and also
will
not give our future open source contributors too much WTF moments when
encountering our style.

On this subject, http://sideeffect.kr/popularconvention/#javascript is
a
good data based source.

We can discuss, add, remove, edit the rules as long as we do not
discuss them
every week!

Other projects:

- Original feross/standard:
After trying it I discovered some rules were not strict enough and
would lead to again style discussions. Has some sort of formatter but
not very good. Based on eslint.

Ref:
- standard/standard#180

- JSCS:
I tried JSCS: good but only oriented on style, no rules on unused
variables or ways to prevent bugs like eslint does. Has some sort of
formatter, but not very good.

Ultimately eslint will have a good formatter, in the meantime we will
all switch to some new coding style, for good :-)?

fixes #128
@vvo vvo force-pushed the chore/use-eslint-config-algolia branch from 1a2a332 to 6aefd52 Compare July 6, 2015 08:08
+ add debugging when it fails
+ close the underlying nodejs keep alive agent when needed (destroy)
@vvo
Copy link
Contributor Author

vvo commented Jul 6, 2015

Imho your PR shouldn't contain the fixes for the CI...

Fixed, merge #135 before this one and we're good

@vvo
Copy link
Contributor Author

vvo commented Jul 6, 2015

Reverted semi colons, fixed html examples. Now gonna write the esformatter for algolia and put it here also as a way to reformat easily our code.

Then will do the same for client-js.

@bobylito
Copy link
Contributor

bobylito commented Jul 6, 2015

And we also have to make the new demos compliant with this :)

@bobylito
Copy link
Contributor

bobylito commented Jul 6, 2015

It fails on Iphone :/

@vvo
Copy link
Contributor Author

vvo commented Jul 6, 2015

Success!

vvo pushed a commit that referenced this pull request Jul 6, 2015
chore: code style change, lint using modified eslint-config-airbnb
@vvo vvo merged commit c692811 into develop Jul 6, 2015
@vvo vvo deleted the chore/use-eslint-config-algolia branch July 6, 2015 19:36
dhayab pushed a commit to algolia/instantsearch that referenced this pull request Jul 10, 2023
…hore/use-eslint-config-algolia

chore: code style change, lint using modified eslint-config-airbnb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use standard formatter
3 participants