Openrefine: Code style improvements

[wetneb]

We have not been enforcing a consistent code style lately. This causes some issues in inconsistent indentation, diverging name conventions, etc.

• [ ] decide which style code should follow (probably broken down by languages)
• [ ] convert the existing code to match these guidelines
• [ ] set up continuous integration to enforce these styles

This would need to be done both for Java and Javascript.

[thadguidry]

"I think", we have some prior discussion on mailing lists...or even old issues, that had some of this discussion (Tom and Stefano had preferences)...but it's 2020 now...so best to start fresh and make decisions on current practices. (just thinking about ECMA and all those debates they themselves had back in the day, hahaha)

[thadguidry]

Actually, yeah, we had some decisions and some of those landed in the /IDEs path

[kushthedude]

@wetneb @thadguidry What about implementing ESLint for JS and Checkstyle for Java >

[thadguidry]

@kushthedude yeah probably Checkstyle for Java...doesn't SonarQube and IntelliJ both use that as a default now? Can you check?

By the way, those are TOOLS, not coding conventions or styles to adhere to, but instead validate against those conventions and styles. I'm more asking about the defaults that they ship out with typically and what the IDEs default use...like Google Java Style, Sun, etc.

UPDATE: Ah, they built the defaults into Checkstyle itself now, so that's cool ... read the headline: https://github.com/checkstyle/checkstyle

[kushthedude]

Can't checkstyle also enforce the coding styles such as Linting, White-Spaces and other Google Java Style Standard Rules?

[kushthedude]

@thadguidry Shall I go ahead implementing the same ?

[thadguidry]

There's a style guide and then there's enforcing compliance of the style.
It sounds like you want to make the enforcement and IDE setup for developers easier?
Optionally enforcing checks on our CI/CD for pull requests?

You can look to a few Apache projects and GitHub projects to see how they do this (only requires us to supply a few extra files historically, but today, I think Eclipse and IntelliJ ship with a few default styles like Google Java Style ...check into this yourself to see. ) http://help.eclipse.org/2019-12/index.jsp
Here's the official location of Google Style Guides for reference (but tools already grab from here): https://github.com/google/styleguide
Eclipse Help setup: http://help.eclipse.org/2019-12/index.jsp

And here's a snippet of instructions that we could also borrow and put somewhere in our docs, perhaps in the CONTRIBUTOR.md or README.md (I don't know...check on how Apache projects set this up and give developers the info, I would say, even though we are not an Apache project, historically we have pulled in their best practices)
https://github.com/HPI-Information-Systems/Metanome/wiki/Installing-the-google-styleguide-settings-in-intellij-and-eclipse#installing-the-coding-style-settings-in-eclipse

So I would say...

1. If we can have a clean way for developers to clone OpenRefine and have their IDE like IntelliJ or Eclipse already setup for Google Code Style, that would be awesome.
2. As well as provide instructions in the README.md or CONTRIBUTOR.md on how to setup the Google Java Style (maybe just links to .
3. For CI/CD, we should look to enable Checkstyle, through GitHub Actions, Travis, and AppVeyor with Maven support.

[kushthedude]

I am planning for developers environment with ci/cd check too.

On Sun, 1 Mar, 2020, 21:07 Thad Guidry, [e-mail] wrote:

There's a style guide and then there's enforcing compliance of the style.
It sounds like you want to make the enforcement and IDE setup for
developers easier?
Optionally enforcing checks on our CI/CD for pull requests?

You can look to a few Apache projects and GitHub projects to see how they
do this (only requires us to supply a few extra files historically, but
today, I think Eclipse and IntelliJ ship with a few default styles like
Google Java Style ...check into this yourself to see. )
http://help.eclipse.org/2019-12/index.jsp
Here's the official location of Google Style Guides for reference (but
tools already grab from here): https://github.com/google/styleguide
Eclipse Help setup: http://help.eclipse.org/2019-12/index.jsp

And here's a snippet of instructions that we could also borrow and put
somewhere in our docs, perhaps in the CONTRIBUTOR.md or README.md (I don't
know...check on how Apache projects set this up and give developers the
info, I would say, even though we are not an Apache project, historically
we have pulled in their best practices)

https://github.com/HPI-Information-Systems/Metanome/wiki/Installing-the-google-styleguide-settings-in-intellij-and-eclipse#installing-the-coding-style-settings-in-eclipse

So I would say...

1. If we can have a clean way for developers to clone OpenRefine and
   have their IDE like IntelliJ or Eclipse already setup for Google Code
   Style, that would be awesome.
2. As well as provide instructions in the README.md or CONTRIBUTOR.md
   on how to setup the Google Code Style (maybe just links to .
3. For CI/CD, we should look to enable Checkstyle, through GitHub
   Actions, Travis, and AppVeyor with Maven support.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/OpenRefine/OpenRefine/issues/2338?email_source=notifications&email_token=AKQMTLREX34ZM5GPVEAB563RFJ6L3A5CNFSM4K5B4VWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENNCNLY#issuecomment-593110703,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AKQMTLTRSDZRS47FKL7DSVLRFJ6L3ANCNFSM4K5B4VWA
.

[thadguidry]

I like a lot of the info that Apache Spark pulls together on their Contributing doc (Code Style is at the bottom of page): http://spark.apache.org/contributing.html
But some of that documentation improvement is already an issue for @ostephens to work on this summer, so I'd say skip that doc improvement for now, we have a grand plan for some of that.

[kushthedude]

I will check it out.

On Sun, 1 Mar, 2020, 21:12 Thad Guidry, [e-mail] wrote:

I like a lot of the info that Apache Spark pulls together on their
Contributing doc (Code Style is at the bottom of page):
http://spark.apache.org/contributing.html

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/OpenRefine/OpenRefine/issues/2338?email_source=notifications&email_token=AKQMTLR74ILUX6YNHYKAC3TRFJ67TA5CNFSM4K5B4VWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENNCRSQ#issuecomment-593111242,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AKQMTLRVTXDQ2ALNPRAVLFLRFJ67TANCNFSM4K5B4VWA
.

[kushthedude]

@wetneb @thadguidry I set up the style enforcements for JS and have used standard js code style using ESLint and I have also set up npm and package.lock for our project. Should I push the changes?

[thadguidry]

@kushthedude Sure, submit with a PR so that we can review as a community.

[thadguidry]

Hi @kushthedude We are awaiting your new PR for the Javascript style. (Then we can work on the CI for the enforcement for that in a separate PR). We want to follow the task steps in the top of this issue.

[kushthedude]

To enforce ESlint there will be a need for node engine. As webjars can't utilise the rules for the ESLint standard js style. Hence first we need to decide do we need to go for yarn/npm or any other alternative? Then we can go for javascript styling.

[wetneb]

@thadguidry I don't think there is any particular rush to get this done - and we should not pressure GSoC/Outreachy applicants to work on more things before the internships start :)

[kushthedude]

@wetneb @thadguidry Do we have to move with Checkstyle for Java ?

[thadguidry]

@kushthedude Antonin as our lead and having to deal with the Checkstyle...and fallout from it... will make that final determination.

[tfmorris]

"I think", we have some prior discussion on mailing lists...or even old issues, that had some of this discussion (Tom and Stefano had preferences)...but it's 2020 now...so best to start fresh and make decisions on current practices. (just thinking about ECMA and all those debates they themselves had back in the day, hahaha)

I think that unless there's a strong reason not to, which should stick with the conventions that were used to write the bulk of the code. Anything else adds a ton of noise.

The conventions were documented in the form of Eclipse project settings that defined things like 4 space indentation, import ordering, etc for Java. For Javascript, I'm not sure we did much more than specify 2 space indents. These got deleted as part of the move to Maven, but I've relocated them and just need to separate out the style preferences from the others.

The biggest issue that I see with new code is the use of tabs for indentation (pure tabs, I think, as opposed to mixed spaces & tabs) instead of the historical 4 spaces. This most likely means that people have their tabs stops set at 4 spaces too, instead of the usual 8, so that things align visually for them.

I have mixed feelings about doing a mass cleanup. This tends to introduce a lot of noise that makes it more difficult to use commit histories and git blame. Whether we do cleanups all at once or incrementally, they should be in separate commits to make it easy to differentiate functional changes and formatting changes.