search

stefan-kaestle / openhab2-addons

Add-ons for openHAB 2.x
Eclipse Public License 2.0
16 stars 1 forks source link

Refactor code to follow official openHAB guidelines #16

Closed coeing closed 4 years ago

coeing commented 4 years ago

The guidelines are here: https://www.openhab.org/docs/developer/guidelines.html

We should follow them as closely as possible.

Here are some first TODOs:

coeing commented 4 years ago

Based on the feedback from Hilbrand (https://community.openhab.org/t/towards-merging-the-new-bosch-shc-binding/101952/2) we should cleanup our code to follow the official guidelines before we create our pull request.

Feel free to add more TODOs. We can also split the task into more issues, it might be a bit of work.

stefan-kaestle commented 4 years ago

Biggest issue might be signing off code. We have to rebase and change the commit messages and the force push.

That's going to be nasty with merges. We already have two, so no idea how to best do this. Maybe git filters are the way to go. https://stackoverflow.com/questions/13043357/git-sign-off-previous-commits

In any case, we need to synchronize here, so that we don't mess up each others work. Maybe we merge the remaining open PRs first? We already have merges, so having a few more shouldn't make the problem worse? :-P

coeing commented 4 years ago

@stefan-kaestle Yes, it might take some work to go through the commits again. I will do it for my pull request branch and maybe @GerdZanker can do it for his as well? If you have time for it, you can adjust the bosch-shc-2.5 branch. If not, let me know, than I can do it :)

The easiest way is to recommit all the commits and adjust the commit message. The push has to be "forced" because changing the commit messages changes the git history. We don't have to merge the pull requests first. It is a widespread practice to rebase pull request branches anyway before a merge, and we can adjust the commit messages during the rebase as well.

coeing commented 4 years ago

Just rebased my pull requests and force-pushed it, there doesn't seem to be any problem with it 👍 https://github.com/stefan-kaestle/openhab2-addons/pull/15

coeing commented 4 years ago

@stefan-kaestle Played around with the bosch-shc-2.5 branch a bit and it shouldn't be a problem there as well. We just should make sure to keep the merge commits so we don't loose the pull request branches (I like that they group the commits that belong to a feature). Other than that it only changes the commit name and email to the one who does the rebase, but the authors of the commits stay the same.

A question that came to my mind: We should probably always use the author of the commit behind the "Signed off by", shouldn't we? On the other hand it might not be correct to sign off a commit for another person. Not sure about it, it's the first time I hear about this sign-off thing :) In this thread it sounds like another person can sign off commits of other authors. In that case the sign-off person have the word of the original author that they have the copyright for the committed code: https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for

I use TortoiseGit as my Git UI, but here are the steps for the sign-off with rebase with vanilla Git: https://stackoverflow.com/questions/25570947/how-to-use-git-interactive-rebase-for-signing-off-a-series-of-commits

coeing commented 4 years ago

Checked correct usage of null annotations and added a pull request for it :)

GerdZanker commented 4 years ago

I'm back from a longer offline periode, starting with a rebase/recommit of the PR #11 including the sign-off.

Regarding

We should probably always use the author of the commit behind the "Signed off by", shouldn't we?

Yes, I think this is best way to avoid questions and confusions and additional work for us to proove to have the contribution right.

GerdZanker commented 4 years ago

Found one more todo in guidelines directory-and-file-layout:

GerdZanker commented 4 years ago

Class documentation is now merged with #21

GerdZanker commented 4 years ago

I reviewed the guide lines

JavaDoc is required to describe the purpose and usage of every:

  1. class,
  2. interface,
  3. enumeration (except inner classes and enums),
  4. constant, field and method with visibility of default, protected or public.

I think we covered the first three quite well, but I have no idea about regarding public & protected constant, field and method JavaDoc. Shall we just await the feedback of the OpenHAB maintainers?

coeing commented 4 years ago

@GerdZanker I would like to close this issue for now, it has already become too long (always happens with general issues). But I'll look for uncommented constants, fields and methods while working on other issues :) The remaining misses should be found during the openHAB review.