Indent rules (at least for js, at least for ATLAS)

Sigfried_Gold · October 12, 2016, 10:58am

Frank just made a pull request for adding jshint to our development environment:

github.com/OHDSI/Atlas

trying to use linters and fixing achilles

OHDSI:master ← OHDSI:linted-achilles

opened 09:00PM - 11 Oct 16 UTC

fdefalco

+2128 -2056

It turns out the issues we were seeing with the legacy achilles (data sources) c…harts was due to some undeclared variables throwing errors once "use strict" was applied on the charting module. I've put a few things into this branch that I wanted to open up for comment before pushing to master. I added .jshintrc which is a preference file for JSHint. JSHint is a javascript code analysis tool. The .jshintrc file will allow us to set conventions for our code that we can then test for compliance. The current .jshintrc is by no means "our accepted standards" just a template I found that seemed well formed. I then made changes to bring the charting module file down from a few hundred JSHint problems to 39. I stopped there as the problems were getting harder to resolve and I wasn't sure if this was a path we would want to adopt. If we are in agreement and get approval on this branch then we can use JSHint (with extensions for brackets, vim, emacs, visual studio, notepad++ and other editors) going forward and make progress on "house cleaning" of our javascript code as time allows. While this doesn't address the broader refactoring we need to do it should help get us moving in the right direction and give us a way to hold ourselves accountable to some quality standard. This branch does fix the data sources modules and reports are now working so I've re-enabled it on the left menu. I also made some style updates to the dropdown buttons for reports and data sources to fix the display issues with the buttons. I also changed so that "use strict" is occurring at a function level instead of module as that was the approach I've seen recommended.

I haven’t tried it yet, but it seems like a great thing. It also reminds me to bring up something that’s been bugging me: indent standards. Apparently – http://jshint.com/docs/options/#indent – jshint isn’t going to help enforce indent rules, but it would be nice if we at least agreed on a standard. I had noticed that the majority of the code used tabs for indent, so that’s what I’ve been doing, and when I come across code that uses spaces, I convert to tabs. I would much prefer that we standardize on 2 spaces for indent. Tabs are a drag when looking at code in github (though there is an option to override the default of displaying them as 8 spaces) or in diffs.

There are still places in the code that are inconsistent (using something other than tabs) or just messed up (not having correct indenting levels). My having cleaned some of these up should make it easier to convert if we do decide on a standard.

What do other people think of two spaces? Any other preferences?

Frank · October 12, 2016, 2:25pm

t_abdul_basser · October 12, 2016, 2:54pm

No preference but +1 on standardizing around one set of standards.

Frank · October 12, 2016, 5:06pm

So the debate on this is long and both “sides” make valid points.

I happen to prefer tabs because I like small code indenting and any IDE I have used allows me to specify the Tab Size (which for me is set to 2). I like the flexibility this gives developers in terms of having the IDE display the tabs at whatever width they prefer.

We aren’t going to get everyone to agree on this but why don’t we take a day and let people reply via this poll and see where we come out.

We should use tabs with flexible IDE controlled tab sizes
We should obviously use two spaces!
Don’t be silly, we need to use four spaces!!

0 voters

Sigfried_Gold · October 13, 2016, 12:10pm

Let me just make the case for spaces a little more clearly than I did above:

In diffs on github, the default is to expand tabs to 8 spaces. So a diff like this:
https://github.com/OHDSI/Atlas/commit/7edca0b2ab689444530cca841fc93c8a36d2da79#diff-4a2749a951896efbff8222ffd9dbd8aeR597 ends up pretty ugly and a little hard to read.

You can use ?ts=2 at the end of github urls to see code files with different tabwidth, but that doesn’t seem to work for diffs.

Frank · October 13, 2016, 1:36pm

I agree that the diff looks awful on github. There appears to be a solution however where we could add a .editorconfig file to the repository that will set an override for how the code is displayed. That being said, I’m not using github for diff.

Sigfried_Gold · October 13, 2016, 1:39pm

Oh, what diff do you use? I mostly see the diff on github when I’m reviewing commits.

Frank · October 13, 2016, 3:07pm

When I’m reviewing what is in commits I’ll use Github Desktop which gives you a view like this:

For diffs I’ll use meld or winmerge or kdiff3.

herrcerd · October 13, 2016, 3:14pm

Have been unable to make the Arch. calls recently owing to conflicting meetings, but wanted to throw my hat in the ring (Poll vote added).

Note - for “auto IDE” adherence - we have found the editorconfig standard (.editorconfig in repo root) to be great for enforcing these rules across the various IDEs our engineers prefer. Have not found a mainstream IDE yet that does not support it.

http://editorconfig.org/