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

Kk/linting+prettier #41

Merged
merged 3 commits into from
Nov 13, 2018
Merged

Kk/linting+prettier #41

merged 3 commits into from
Nov 13, 2018

Conversation

krzkaczor
Copy link
Contributor

@krzkaczor krzkaczor commented Nov 10, 2018

What's inside:

  • autoformatting with prettier (in few places in tests we used fixtures and I used // prettier-ignore to ignore these pieces b/c they are looking better in single lines)
  • tslint ruleset
  • npm run test runs linting + tests
  • npm run test:fix runs linting in autofix mode
  • use test:fix during development and test on CI

Notes:

  • Prettier uses similar settings as standardjs (personally i dont care about semicolons and quotes)
  • Prettier was used to format MD files into consistent format as well
  • Tslint disallows var. const is prefered but you can use let when reassignment is needed
  • We can enfore even more rules with tslint - please add you comments

@coveralls
Copy link

coveralls commented Nov 10, 2018

Coverage Status

Coverage increased (+0.1%) to 87.826% when pulling a747c77 on kk/linting+prettier into d1272cc on typescript.

@GrandSchtroumpf
Copy link
Contributor

Personally, I'm in favor of semicolons at the end of a statement. Especially with Typescript, which aimed to help C# engineers to write Javascript code.

@holgerd77
Copy link
Member

@GrandSchtroumpf @krzkaczor Ah, sorry, that's one of the few things where I can't make any compromises. We have non-semicolon code style in all our libraries, and this is a highly opinionated thing and also not really related to TypeScript (one very well finds articles on both sides when googling for best practices), so it wouldn't be fair to side-introduce this here and overrule the implicit and explicit convention of various contributors who have agreed on this.

If someone on the current team is reading this, please also feel free to comment!

.prettierrc Outdated Show resolved Hide resolved
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Ok, I am through with a first round, discussioning the explicitly introduced options.

We should do a second iteration here and:
a) Also have at least some look on the TypeStrict (what a name 😄) settings implicitly introduced here
b) have one thought/fly over the rules not introduced in this PR (so I would encourage everyone to have some look at the rules doc pages for Prettier and TSLint and have some one-step-back thought on what would eventually make sense)

.prettierrc Show resolved Hide resolved
.prettierrc Outdated Show resolved Hide resolved
.prettierrc Outdated
"semi": false,
"singleQuote": true,
"trailingComma": "all",
"proseWrap": "always"
Copy link
Member

Choose a reason for hiding this comment

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

I just switched my own behavior (actually last week 😄) to not introduce line-breaks in md or rst any more, since this becomes less readable e.g. on mobile devices and one can always activate soft-wrap on editor, so I would be in favor not to break, no strong opinion on this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesnt change the rendered output though right? Could you play with these settings locally and let me know what do you prefer? https://prettier.io/docs/en/options.html#prose-wrap

Copy link
Member

@holgerd77 holgerd77 Nov 13, 2018

Choose a reason for hiding this comment

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

Just tried locally, the current setting wraps markdown on the (extended 😄) print width. E.g. on my laptop (13 inch) editor window this already gives super-suboptimal results, so please let's remove this and keep the default as-is setting.

I am just realizing that THIS is actually what this printWidth setting from above is about (I have interpreted this as just being for printing out code on a printer, hehe 😄). So this already clashes with my personal setup. I know that I am not the norm, but I think it should be possible to pretty-view the code on a relatively standard setup (13'' MacBook Air, Atom Editor, Full Screen), and Atom is actually also by standard displaying this 80 characters line.

So on a second thought I would want to change that, either keep the default and let the outer system decide on when to go larger here, or we could maybe go 10 characters longer if this helps. But this setting is killing it for too many people working on a laptop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay i removed this setting

tslint.json Show resolved Hide resolved
tslint.json Show resolved Hide resolved
tslint.json Show resolved Hide resolved
tslint.json Show resolved Hide resolved
"prefer-const": true,
"no-var-keyword": true,
"interface-name": [true, "never-prefix"],
"no-commented-code": true,
Copy link
Member

Choose a reason for hiding this comment

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

Again, don't find this rule in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

If these rules are defined "somewhere" around in the ecosystem, how come that they are included in the generic TS code base? Or did you add dependencies 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.

These additional rulesets are part of typestrict so tslint just finds them 🤷‍♂️ I agree that we could add them to dev deps as well...

Copy link
Member

Choose a reason for hiding this comment

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

Generally I think this rule make sense, commented out code (side node: this rule would be self-descriptive if just named "no-commented-out-code", just another reminder on how important names are) tends to very much be left and float around forever and putting additional noise to the code base.

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, don't let's bloat up the dev dependencies on this, but we should find ways to make the fact transparent that rules are also coming from there.

Actually I will start on an organizational ReadTheDocs documentation on the organization repo in the coming days, including things like contribution guidelines and also some tech standards.

I will also include a TypeScript section there and try to extract from here the common set of practices and tools we agreed upon. This guide can the referenced from the various repos.

Copy link
Member

Choose a reason for hiding this comment

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

(ah, and maybe to the point: we can add some additional explanation on TypeStrict there as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I will update the readme of typestrict.

tslint.json Show resolved Hide resolved
@krzkaczor
Copy link
Contributor Author

krzkaczor commented Nov 13, 2018

@holgerd77 just to clarify, I am the creator of TypeStrict (i love the name 😆). Idea behind this project is to bring rules that are hard to argue with — not enforcing the code style but rather finding bugs. I am happy to discuss any chosen rule, they are described in the readme.

.prettierrc Outdated
@@ -0,0 +1,7 @@
{
"printWidth": 120,
Copy link
Member

Choose a reason for hiding this comment

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

Can we please change the setting here as well (see my comment below on proseWrap)?

Copy link
Member

Choose a reason for hiding this comment

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

(ah, you are probably in-the-works, seems you are making very incremental pushes 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Soooo, should i make it 80 then? what about 100 at least 🤣

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks so much Chris for the work done here!

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.

None yet

4 participants