Inconsistent code style

[ajs124]

I'm taking a look at several parts of this project right now and noticed that there are lots of whitespace errors, e.g. indented empty lines and spaces at the end of lines and inconsistencies in code style, e.g. tabs and spaces randomly mixed, even in the same file.

While this isn't a big problem, it means at least two things (to me)

• it's unprofessional. As harsh at it sounds, if a project cannot get these surface level things right, why would it get the actual code right?
• it's confusing. If I'm contributing code, what should it look like? How should it be formatted, structured, etc.

Assuming you agree to this in principle, the question is how to solve this. Reformatting old code is annoying, because it leads to huge "useless" commits which make version control history harder to use. For new code or code that's being touched anyways, it's worth thinking about trying to enforce some kind of standard. There are projects like editorconfig which aim to address this accross different IDEs and editors while also having CI integration with GitHub actions or other systems.

Therefore my proposal would be to leave the old code as-is and introduce an editorconfig and have CI validate against it on PRs.

[relu91]

I do agree that we need a proper code style check for node-wot.

Reformatting old code is annoying, because it leads to huge "useless" commits which make version control history harder to use.

Well, why not make one commit and refactor the whole repository? I think it does not hurt too much the history and the dev experience.

About enforcing code style my favorite solution is to use Husky. It adds a pre-commit hook in your package.json where you can run your favorite code prettier. Like so, it is almost impossible to commit not formatted code. The downside is that you add a dev dependency.

footnote: In my PRs I used the codestyle script that is provided inside of some binding-* module. There it uses typescript-standard as a code formatter. We could continue using that one.

[danielpeintner]

Therefore my proposal would be to leave the old code as-is and introduce an editorconfig and have CI validate against it on PRs.

While I agree with most of your technical arguments I disagree with the way it was communicated. Either way, let's look at the technical arguments:

• we do have editorconfig
• wo do NOT have PR check

Well, why not make one commit and refactor the whole repository? I think it does not hurt too much the history and the dev experience.

On the hand I agree (since it solves all issues at once) on the other hand I disagree (given that it makes code parts coming from person X while being from Y).

Other than that I see 2 next steps forward now

1. Agree on a style guide and make it easy for developers to use it and/or be aware of it
   • We do have .editorconfig already and it should automatically kick in many Editors (e.g., VS Code) but it seems we still have source code parts that do not respect it properly
   • We need to mention this styleguide more explicitly since people seem to miss it
2. check each new PR for the rules (we use Travis at the moment)

   • About enforcing code style my favorite solution is to use Husky. It adds a pre-commit hook in your package.json where you can run your favorite code prettier.

   • @relu91 would you have already a proven project/code snippet that we could use?
   • personally I would like to have another Travis check that checks styleguide similar to the Eclipse ECA check we have now. Does anyone have experience with it?

[danielpeintner]

I think it should be as simple as adding the following lines to .travis.yml

before_script:
  - npm install -g eclint
  - eclint check * "lib/**/*.js"

Note: we need .ts et cetera ...

see https://github.com/jedmao/eclint

[relu91]

on the other hand I disagree (given that it makes code parts coming from person X while being from Y)

I see I did not consider this issue... however, it should be possible to find the original author from history. This consideration is also true for almost all trivial reactors (i.e. like moving a script inside another directory).

We do have .editorconfig already and it should automatically kick in many Editors

Nice! so I think we should also remove the codestyle scripts inside package.json. As I mentioned in the previous comment it uses typescript-standard and it may use different rules for formatting. Then, we could use eclint fix ... instead.

@relu91 would you have already a proven project/code snippet that we could use?

Please see how we are using it inside wam. It adds the recommit hook where you can run your favorite code formatter. However, if you are not going to format everything it might commit unwanted lines of code. So the use of this tool should be limited only when the whole codebase is already formatted following the rules.

I think it should be as simple as adding the following lines to .travis.yml

before_script:

- npm install -g client

- eclint check * "lib/**/*.js"

Note: we need .ts et cetera ... see https://github.com/jedmao/eclint

Never tested but it seems fine!

[ajs124]

I disagree with the way it was communicated

I'm not much of a writer (and obviously not an English native speaker), sorry if anyone felt attacked by my phrasing.

And I definitely shouldn't have missed the .editorconfig, since I was advocating for it :sweat_smile:

Decisions on which solution to choose and if you want to reformat old code are obviously up to you. I don't have many arguments for one or the other, although from personal experience, running git blame and always finding the same commits which "do nothing" can be quite irritating. Well, that and commits which just dump tens of thousands of lines of code.

My overall goal with my work for @egekorkan is to compare this project with some others and also try and make it more approachable and easier to contribute to, while I'm at it. I hope this is in your interest and future communications from me will hopefully be less "disagreeable". If they still are, feel free to point it out again.

[relu91]

Decisions on which solution to choose and if you want to reformat old code are obviously up to you. I don't have many arguments for one or the other, although from personal experience, running git blame and always finding the same commits which "do nothing" can be quite irritating. Well, that and commits which just dump tens of thousands of lines of code.

I do agree that such a kid of commit is not a nice thing to see in history. However, sometimes we have to sacrifice something for the greater good 😄 . Anyway, I am open to other solutions. I think that checking every new PRs on Travis will eventually put the code format to the state that we all want. It is just a longer process. 👍

[danielpeintner]

I think that checking every new PRs on Travis will eventually put the code format to the state that we all want. It is just a longer process. 👍

I am also unsure about "what" is the right way forward. Having one bigger clean-up fix or many fixes over the time. The only issue I see is that IF we use eclint check without the initial clean-up fix it might fail till all issues are resolved. Meaning that it checks all files and not only the newly committed files. I need to check this behavior but I am not sure when I find the time to do so...

[danielpeintner]

There is new linter with nice Github Actions support, see https://github.com/github/super-linter

Does it make sense to change from Travis builds to Github Actions?

[egekorkan]

Although it may make sense, GitHub Actions is not really free. You have 2000 minutes per month whereas Travis gives unlimited minutes. If the only reason for GH Actions is the linter, I think it would be unnecessary work and not futureproof.

[relu91]

I agree with Ege, also do we need a general-purpose linter such as super-linter? Or we might live up well with just eslint or standard or .editorconfig?

[danielpeintner]

I looked once again at EditorConfig. There is built-in Support in "Visual Studio" but NOT in "Visual Studio Code". Having said that, one needs to look for the "EditorConfig for VS Code" extension and install it.

Do we want to require that? Note: the default Visual Studio Code formatting seems to differ, especially w.r.t. nesting

BTW, one can also use any other technique to achieve the same format settings defined in .editorconfig e.g., eclint fix **/*

FYI: I created PR https://github.com/eclipse/thingweb.node-wot/pull/261 to show the huge amount of changes we run into IF we do it with one commit

[danielpeintner]

An addition could https://lgtm.com/#pull_requests

[danielpeintner]

We have codestyle and format checkers in place now.