Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A check for author tag is implemented. Some tests for it are added. #52

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

m-memova
Copy link

@m-memova m-memova commented Apr 5, 2017

Signed-off-by: Mihaela Memova [email protected]

@svilenvul
Copy link
Contributor

Great, thanks, please update the README.MD file with the priority of the check.

@m-memova
Copy link
Author

m-memova commented Apr 5, 2017

@kaikreuzer , are you OK with this check failing the build or should I change its priority to "warning"?

@kaikreuzer
Copy link
Member

Failing the build is fine, thanks!

@m-memova
Copy link
Author

m-memova commented Apr 5, 2017

Done! :)

@m-memova
Copy link
Author

m-memova commented Apr 5, 2017

So, I tried this check in the addons2 repository. And it turned out that more than 100 classes have no author tags. That means that when this PR is merged, the build will fail for every new PR. So I searched through all the bindings and I made a list of potentials authors. I think it would be nice to make an issue here. If the authors add their tags before merging this PR, the build will be successful. @kaikreuzer, @svilenvul , what do you think?

@kaikreuzer
Copy link
Member

I fear that it will take forever to ask the individual authors to add that.
Question: Is it easily possible to deactivate certain rules on certain bundles? Then we could add an "ignore" to the pom.xmls of the concerned bundles and still would have the check in place for any new contribution.

@m-memova
Copy link
Author

m-memova commented Apr 6, 2017

I can add all the files-with-no-author tag in the suppressions.xml file. But I am not sure that would be a better decision. Besides, there are several things to consider :

  • 71 of those files are from org.openhab.ui.cometvisu (judging by the rest of the files there, we can be pretty sure that the author is Tobias Bräutigam)
  • many of the files with no author tag are from internal packages (according to the ESH guidelines every file must have an author tag but still...)
  • there are some files which have an author tag but the comment is not java-doc style and I can easily fix it.

@kaikreuzer
Copy link
Member

71 of those files are from org.openhab.ui.cometvisu (judging by the rest of the files there, we can be pretty sure that the author is Tobias Bräutigam)

I just quickly scanned through this bundle and only saw a single class where the tag was missing. Do you have details?

many of the files with no author tag are from internal packages (according to the ESH guidelines every file must have an author tag but still...)

There might be SOME exceptions, like e.g. generated code (e.g. JAXB beans) or third-party code that has been included in the repo - both are valid exceptions to the rule, so for those cases it would be nice if there is a chance to configure the bundle in a way that this check is suppressed.

@martinvw
Copy link
Member

martinvw commented Apr 6, 2017

@kaikreuzer we can of course track back the authors by doing some git blames, or is that not sufficient.

@kaikreuzer
Copy link
Member

@martinvw That won't help for the two exceptional cases that I mentioned (generated & 3rd party code).

@m-memova
Copy link
Author

@kaikreuzer , you are right, 70 of the files with no author tag in org.openhab.ui.cometvisu are generated by the JavaTM Architecture. I will think about how we can handle those exceptions of the rule.

@martinvw
Copy link
Member

In most projects I worked on professionally we put the generated code in the target directory and let maven generate it on each build, we could try whether that is a possible option here too. It would solve this problem and make sure the generated code is not accidentally altered

@kaikreuzer what would you think about this?

@svilenvul
Copy link
Contributor

I think for now we can just add them to the suppression filter. (since most of the files are in a single bundle).
@m-memova, what do you think about this ?

@kaikreuzer
Copy link
Member

we put the generated code in the target directory and let maven generate it on each build

I only do this for code that we do not compile against ourselves - the rest is checked in, because otherwise you will have hundreds of compilation errors after doing the IDE setup.
This is the beauty of the OH2 IDE setup over the ESH IDE: It does not require any additional code generation steps and thus is much simpler to get started with.

@martinvw
Copy link
Member

This is the beauty of the OH2 IDE setup over the ESH IDE: It does not require any additional code generation steps and thus is much simpler to get started with.

Maybe that is more something to debate over beer, but I would not consider it beauty that you lose track of the difference between generated code and potentially customized (generated) code :-)

@kaikreuzer
Copy link
Member

Yes, I go for the beer 🍻

@svilenvul
Copy link
Contributor

I am sure that all of us would like to see this check merged :-), so I will start resolving some of the errors. I still have some questions :

  • there are some bundles that contain one ore two classes that miss the author tag. Is it OK to add the authors using the information from git blame?
  • there are some auto generated classes. Is it possible to move them to src/gen/java source folder and ignore them directly in the check, or they will be added in the exclude filter ?
  • there are some multi line comments that should be changed to javadoc;

@martinvw
Copy link
Member

martinvw commented Jun 9, 2017

there are some auto generated classes. Is it possible to move them to src/gen/java source folder and ignore them directly in the check, or they will be added in the exclude filter ?

I think that that is great solution

svilenvul pushed a commit to MusalaSoft/openhab2-addons that referenced this pull request Jun 9, 2017
svilenvul pushed a commit to MusalaSoft/openhab2-addons that referenced this pull request Jun 9, 2017
svilenvul pushed a commit to MusalaSoft/openhab2-addons that referenced this pull request Jun 12, 2017
kaikreuzer pushed a commit to openhab/openhab-addons that referenced this pull request Jun 13, 2017
svilenvul pushed a commit to MusalaSoft/openhab2-addons that referenced this pull request Jun 13, 2017
Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
svilenvul pushed a commit to MusalaSoft/openhab2-addons that referenced this pull request Jun 13, 2017
Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
Some tests for it are added.
Added filter for auto generated classes.

Also-by: Svilen Valkanov <[email protected]>
Signed-off-by: Mihaela Memova <[email protected]>
kaikreuzer pushed a commit to openhab/openhab-addons that referenced this pull request Jun 13, 2017
Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
@svilenvul
Copy link
Contributor

The cleanup is done :).

Are we ready to merge this check as well ?

@svilenvul svilenvul merged commit b8f1281 into openhab:master Jun 14, 2017
falkena pushed a commit to falkena/openhab-addons that referenced this pull request Jun 29, 2017
falkena pushed a commit to falkena/openhab-addons that referenced this pull request Jun 29, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
qvistgaard pushed a commit to qvistgaard/openhab2-addons that referenced this pull request Jun 30, 2017
qvistgaard pushed a commit to qvistgaard/openhab2-addons that referenced this pull request Jun 30, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
jsjames pushed a commit to jsjames/openhab-addons that referenced this pull request Jul 1, 2017
jsjames pushed a commit to jsjames/openhab-addons that referenced this pull request Jul 1, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
reyem pushed a commit to reyem/openhab2-addons that referenced this pull request Jul 1, 2017
reyem pushed a commit to reyem/openhab2-addons that referenced this pull request Jul 1, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
Markinus pushed a commit to Markinus/openhab2-addons that referenced this pull request Jul 2, 2017
Markinus pushed a commit to Markinus/openhab2-addons that referenced this pull request Jul 2, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
ppieczul pushed a commit to ppieczul/openhab2-addons that referenced this pull request Jul 2, 2017
ppieczul pushed a commit to ppieczul/openhab2-addons that referenced this pull request Jul 2, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
reyem pushed a commit to reyem/openhab2-addons that referenced this pull request Jul 3, 2017
aogorek pushed a commit to aogorek/openhab2-addons that referenced this pull request Jul 5, 2017
aogorek pushed a commit to aogorek/openhab2-addons that referenced this pull request Jul 5, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
aogorek pushed a commit to aogorek/openhab2-addons that referenced this pull request Jul 5, 2017
Markinus pushed a commit to Markinus/openhab2-addons that referenced this pull request Sep 8, 2017
Markinus pushed a commit to Markinus/openhab2-addons that referenced this pull request Sep 8, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
hillmanr pushed a commit to hillmanr/openhab2-addons-pollytts that referenced this pull request Dec 6, 2017
hillmanr pushed a commit to hillmanr/openhab2-addons-pollytts that referenced this pull request Dec 6, 2017
…ab#2364)

Includes:
- auto generated classes moved to src/gen/java;
- added author tag to PHPProvider;

See openhab/static-code-analysis#52

Signed-off-by: Svilen Valkanov <[email protected]>
aogorek pushed a commit to aogorek/openhab2-addons that referenced this pull request Dec 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants