code base clean up

[connectdotz]

motivation:

part of 4.0 initiative

we had some inconsistent eol problem, some are LF, and the other CRLF, which made GitHub diff going crazy... And since we upgraded prettier there are new defaults, such as Arrow Function parentheses that made our code inconsistent as well. Therefore, I think it is best to run prettier for the whole project to bring it up to a consistent base.

summary

• run prettier --write for the whole code base
  • moved the prettier rule to separate config file and stop repeating the same rules everywhere
  • standardize eol (LF) for all platforms when checking into github (added .gitattributes ).
  • added prettier check to ci
• migrate from tslint to eslint and fixed lint errors
  • there are still many warnings I didn't have time to fix. The future development could fix those as we come across them
• convert to semi-colon

test

besides the normal unit test, I also did some simple test with my local workspaces, they seem fine.

[coveralls]

Pull Request Test Coverage Report for Build 772

• 763 of 831 (91.82%) changed or added relevant lines in 33 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 90.523%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/DebugConfigurationProvider.ts	21	22	95.45%
src/TestParser.ts	1	2	50.0%
src/messaging.ts	12	13	92.31%
src/Coverage/CoverageOverlay.ts	18	20	90.0%
src/diagnostics.ts	33	35	94.29%
src/extension.ts	14	16	87.5%
src/TestResults/TestResultProvider.ts	52	55	94.55%
src/helpers.ts	26	29	89.66%
src/extensionManager.ts	63	67	94.03%
src/StatusBar.ts	106	114	92.98%

Totals
Change from base Build 757:	0.03%
Covered Lines:	920
Relevant Lines:	992

---

💛 - Coveralls

[connectdotz]

unbelievable, the eol is tricky, took me quite a few rounds to get it working (hopefully) for Travis windows env. @stephtr when you get a chance please see if there is any unexpected consequence on windows. If you have git core.autocrlf set to true, then you probably won't be able to run yarn prettier-project-check as your checkout will be converted with windows CR-LF, not sure if we should force all checkout to LF (the default for Mac and Linux)?

Now there are so many files touched, made me wonder maybe it is more efficient to just do the rest "non-code" mass-migration (adopting semicolon and tslint=>eslint), so we get it over in one shot instead of multiple PRs/reviews... @seanpoulter @stephtr do you guys have any objection?

BTW, I assume we are all ok switching to semicolon, but figure I should confirm again...

[seanpoulter]

Nice work! 👏

There's a few lines that are getting close to that 120 character limit I object to but it isn't worth any effort to debate code formatting. If you're happy, we're happy. The one thing to point out is this may affect our 4 open PRs. It's a small risk.

[connectdotz]

@seanpoulter thanks for taking the time to review! 👍

There's a few lines that are getting close to that 120 character limit I object to but it isn't worth any effort to debate code formatting.

I agree the auto-formatting sometimes looked silly. I figure developers can always change it later if it bothers them, provided prettier check passes.

The one thing to point out is this may affect our 4 open PRs.

there is actually only 1 "active" PR, which is small, so hopefully, merge/rebase should not be too bad...

I will merge this in, as there is another large-scale baseline change coming up: semicolon and eslint migration... then we can start to work on the more interesting problems in v4...

I didn't hear your opinions regarding semicolon, I take it as "no objection"? In that case, I will pull the trigger in the next PR...

[seanpoulter]

Sounds good for the PRs then.

I have no objection to semi-colons. I haven't heard if Babel or TypeScript have solved the ambiguity in the grammar that Douglas Crockford has complained about. It was something like:

function f() {
  return
  {
    key: 'value'
  }
}

// f() => undefined

I'm happy to review smaller MRs if you get a chunk of work done every day or so.

[connectdotz]

didn't merge as I originally proposed... thinking it might be better to put all these internal changes in one PR so other PRs/developers don't have to merge/rebase it multiple times...

I'm happy to review smaller MRs if you get a chunk of work done every day or so.

I will commit the rest of the changes in multiple commits within this PR, so you can do progressive reviews, knowing you have seen the priors... hope that will work. @seanpoulter once again, thanks for helping and pushing forward the v4!

[seanpoulter]

Can I suggest another strategy? Since you're motivated and driving these changes for v4, why don't we prioritize keeping you unblocked. I find big PRs are hard to find time to review, so I'd say get this merged.

We could use a long-lived "v4 work in progress" branch but that's also going to slow you/us down if we have any PRs go in. Again, if we're prioritizing keeping you unblocked this isn't the best approach.

What about our PRs? Most of these broad changes are (hopefully) automated. That means we should be able to revive our PRs with a process like:

• Update the PR branch to the point right before we merge this
• Checkout the code formatting and code mod configuration and version info from the HEAD of master
• Install and run the tools
• Update the PR branch to the latest (HEAD of master)

I think that subset of files looks like:

• .gitattributes
• .travis.yml
• jsconfig.json
• package.json (the part that isn't formatting)
• prettier.config.js

If that sounds good, I'll move this into a living document that we can all edit (in the Wiki?).

What do you think?

[connectdotz]

@seanpoulter sorry just saw your reply after the eslint migration commit...

I would admit that I do not fully grasp the strategy you are suggesting...? yes large change is harder to review, but most of the changes here are sort of "mechanical", should not have much logic implication, hopefully, should be easier to glance over for the most part.

I plan to finish this PR this weekend, after that there should be no "massive" code change, just regular smaller scale PRs probably. Knowing this, can you elaborate on your proposal? I definitely like the idea to have a shorter cycle of code change, discussion, review... how do we make that happen?

[seanpoulter]

Ah, yea, more code. 🤷

I was suggesting we should merge what's already approved and keep the PRs separate. That way it's easier for me to review what's changed, and easier for anyone else to review the next MR without having to tackle the whole thing.

[connectdotz]

Ah, yea, more code. 🤷

hahaha, that was funny, nice emoji!

Sorry that I misunderstood your point, but hey, you only have to look at them, I have to do the actual work to research and write (well, lost of copy/paste actually)... ok, seriously, I will make you a deal, do this review and I won't make you do the semi-colon review, which is pretty safe to just check-in as long as it passed all the tests.

That way it's easier for me to review what's changed

You can see diff between 2 commits, for example, to see the eslint migration change vs the previous commit that you have reviewed: 7e8e3a6^...7e8e3a6

Migration to eslint is actually a fun process, I learned a few new things along the way, for example, some of our tests "cheated" and that they actually did not work 😂 ; It is impossible to partially mock module function declarations like in our TestResults.ts. There is an interesting jest discussion about it. I ended up converting the (public) declarations to expressions in order to properly mock them... anyway, I hope you will have as much fun reviewing the code...

[connectdotz]

@seanpoulter just wondering if you get a chance to look at the last commit yet, I was waiting to hear from you before proceeding to the semi-colon...

[seanpoulter]

Hey @connectdotz, no, sorry. I gave it a quick skim but haven't reviewed the non-obvious changes like the tests that you mentioned. Feel free to add it as another commit or PR against this branch?

[seanpoulter]

@seanpoulter just wondering if you get a chance to look at the last commit yet, I was waiting to hear from you before proceeding to the semi-colon...

Done. There's no blocking comments. Nicely done.

[connectdotz]

decided to wrap prettier under eslint, so all format errors will be shown just like lint error. Together with eslint's auto fix on save and ci check, don't think we need lint-staged during pre-commit anymore, so it is gone too...

I think the package.json looked reasonably clean now. If no objection, I am going to pull the trigger for the following format changes (prettier):

• semi: true
• printWidth: 100
  • I was thinking about @seanpoulter comment about 120 is too long and 80 is too short sometimes, so let's split in the middle then?

[connectdotz]

I think I have touched enough files in this PR 😱 Ready to merge now. Last call for questions/concerns... @seanpoulter @stephtr

[seanpoulter]

Approving again after skimming the last 4 commits. That's a lot of code churn. 🤣

Thanks for that effort @connectdotz! 👏