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

Removed the brace-style ESLint exclusion #1434

Merged
merged 8 commits into from
Aug 15, 2017
Merged

Removed the brace-style ESLint exclusion #1434

merged 8 commits into from
Aug 15, 2017

Conversation

dbemiller
Copy link
Contributor

Type of change

Code style update (formatting, local variables)

Description of change

Fixing one of the linter exceptions for #1206.

Everything was done with lint --fix except for the following three spots, which I fixed manually:

dbemiller-mac:Prebid.js dbemiller$ ./node_modules/eslint/bin/eslint.js src modules/ --fix

/Users/dbemiller/code/Prebid.js/src/adloader.js
  45:3  error  Closing curly brace does not appear on the same line as the subsequent block  brace-style

/Users/dbemiller/code/Prebid.js/src/bidmanager.js
  230:3  error  Closing curly brace does not appear on the same line as the subsequent block  brace-style

/Users/dbemiller/code/Prebid.js/modules/rhythmoneBidAdapter.js
  124:63  error  Closing curly brace does not appear on the same line as the subsequent block  brace-style

@dbemiller dbemiller changed the title Fixed the curly brace styles Removed the brace-style ESLint exclusion Jul 31, 2017
{ callback(200, 'success', response.responseText); }
else
{ callback(-1, 'http error ' + response.status, response.responseText); }
if (response.status === 200) { callback(200, 'success', response.responseText); } else { callback(-1, 'http error ' + response.status, response.responseText); }
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 harder to read than before. It doesn't look like standardjs implements a max-len but I think we should consider it. I guess it doesn't do that because it can't --fix that automatically?

Copy link
Contributor Author

@dbemiller dbemiller Aug 8, 2017

Choose a reason for hiding this comment

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

I'm not sure why standardjs doesn't define one... but I agree, this is harder to read this way. For now I'll split it up manually.

We should consider max-len in the future. In my experience that rule ends up being ~80% good, 20% bad. It prevents code like this, which is great, but it can be a negative if you want to write long error messages or string constants.

I'd still be in favor of it though. You can always disable the linting rules for your line, if you have a really good case for it.

@mkendall07 mkendall07 merged commit d13d3eb into master Aug 15, 2017
@mkendall07 mkendall07 deleted the fix-brace-style branch August 15, 2017 20:30
philipwatson pushed a commit to mbrtargeting/Prebid.js that referenced this pull request Sep 18, 2017
* Fixed the curly brace styles.

* Merged from master and fixed the conflicts.

* Merged from master, and re-fixed the style issues.

* Improved the style.

* Fixed style issue from master.
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
* Fixed the curly brace styles.

* Merged from master and fixed the conflicts.

* Merged from master, and re-fixed the style issues.

* Improved the style.

* Fixed style issue from master.
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Fixed the curly brace styles.

* Merged from master and fixed the conflicts.

* Merged from master, and re-fixed the style issues.

* Improved the style.

* Fixed style issue from master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants