Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Updated linter line length to something less draconian. #275

Merged
merged 2 commits into from
Sep 15, 2017

Conversation

blackbaud-brandonhare
Copy link
Contributor

No description provided.

tslint.json Outdated
@@ -82,7 +82,7 @@
],
"max-line-length": [
true,
100
200

Choose a reason for hiding this comment

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

I'm going to disagree here, but will defer to @Blackbaud-BobbyEarl. The line length might seem too short on a widescreen monitor, but shorter lines are easier to read and enforce fewer tab structures.

Obviously this is opinion based, but I think the consensus is 80-100 characters.

Interesting read: https://stackoverflow.com/questions/110928/is-there-a-valid-reason-for-enforcing-a-maximum-width-of-80-characters-in-a-code

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 understand the rationale, but for me personally, it is really obnoxious with strings. I don't think I should have to modify my string literals to be concatinated or array.joined to pass a linter rule. There has to be some kind of happy medium between the two.

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 could also pepper my code with //Ignore rule, but that seems just as bad.

Choose a reason for hiding this comment

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

const str = [
  'a',
  'long',
  'string'
].join('');

^ To me, that's a thing of beauty :)

Choose a reason for hiding this comment

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

I'm a huge proponent of longer lines than we allow today. I generally fit 120-140 characters per line without scrolling on my laptop.

@codecov-io
Copy link

codecov-io commented Sep 15, 2017

Codecov Report

Merging #275 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #275   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          57     57           
  Lines        1344   1344           
  Branches      200    200           
=====================================
  Hits         1344   1344
Flag Coverage Δ
#builder 100% <ø> (ø) ⬆️
#runtime 100% <ø> (ø) ⬆️
#srcapp 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcc15e0...8126893. Read the comment docs.

@Blackbaud-LuisBello
Copy link

i endorse this pr 🇺🇸

@Blackbaud-PaulCrowder Blackbaud-PaulCrowder merged commit f304995 into master Sep 15, 2017
@Blackbaud-PaulCrowder Blackbaud-PaulCrowder deleted the hotfix-linter-line-length branch September 15, 2017 21:02
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.

6 participants