You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openmeetings.apache.org by GitBox <gi...@apache.org> on 2020/04/26 22:17:15 UTC
[GitHub] [openmeetings] sebawagner opened a new pull request #69: OPENMEETINGS-2307 Add checkstyle check with only checking for unused …
sebawagner opened a new pull request #69:
URL: https://github.com/apache/openmeetings/pull/69
Enables checkstyle with 1 checkstyle check:
* Unused Imports (https://checkstyle.sourceforge.io/config_imports.html#UnusedImports)
It will fail at compile time for this one. I removed a few unused imports that checkstyle found.
I also enabled the report. But since the only check we have will error at compile time the report will be empty, cause it will fail before generating the report.
However we may add more checks later that are just warnings. Then the report becomes more useful.
Please have a look what other modules/checks you would like to enable as error or warnings:
https://checkstyle.sourceforge.io/checks.html
For instance:
* https://checkstyle.sourceforge.io/config_whitespace.html#FileTabCharacter can be quite annoying
* https://checkstyle.sourceforge.io/config_misc.html#Indentation format code
This can save a lot of time when reviewing a PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #69: OPENMEETINGS-2307 Add checkstyle check with only checking for unused …
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#discussion_r415413887
##########
File path: openmeetings-core/pom.xml
##########
@@ -32,6 +32,7 @@
<properties>
<site.basedir>${project.parent.basedir}</site.basedir>
<autoModuleName>apache.openmeetings.core</autoModuleName>
+ <checkstyle.config.location>${project.parent.basedir}/src/config/openmeetings_checkstyle.xml</checkstyle.config.location>
Review comment:
Needs to overwrite config location inherited from parent. Parent can't configure ${project.parent.basedir} as config as it has no parent. Its the same problem like with site.basedir overwrite 2 lines above. I think this seems to work fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619790357
I can't "approve" my own pull request :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619722678
I think you can just add more file extensions it will go through. I just configured java for now. I think it can handle xml files.
But honestly for .js files I would rather recommend to use a linter.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619910830
I know but still I don't think there is any real alternative in the Java
world.
It's also quite a difficult task: Write a code that understand a code
language and is configurable AND do that across different languages.
Checkstyles doesn't just apply regexp it reads the code and tries to
understand the syntax and logic.
It's just never been intended to use it for with other then Java. But for
Java it's really good.
Thx
Seb
On Mon, 27 Apr 2020, 10:01 PM Maxim Solodovnik, <no...@github.com>
wrote:
> @sebawagner <https://github.com/sebawagner> "Trailing space" checker is
> outside TreeWalker and it doesn't work with files other than *.java
> One of the most weird plugin I ever use ....
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/openmeetings/pull/69#issuecomment-619874905>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABHJ2QD3SXF3EHKFDMEOUP3ROVJY5ANCNFSM4MRNXWXQ>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619660199
It should be also possible to publish check-style report on Jenkins. So that you can see the report right away.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619705692
I have added some changes @sebawagner please review
As I wrote before I found no way to add checks for non-java files
this plugin - is something non-configurable ....
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619703611
Hello @sebawagner,
I, personally, don't like check-style plugin :(
According to my experience it is near to impossible to be configured in non-default way
Already spent ~2 hours on it - seems to be simple time killer :(
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#discussion_r415515187
##########
File path: pom.xml
##########
@@ -1084,6 +1122,22 @@
</reportSet>
</reportSets>
</plugin>
+ <!-- plugin>
Review comment:
I think delete this as its commented out and the merge.
I don't mind the reporting side. But obviously having some warnings would be useful as overall report.
This could be also a good point to give to "novice" developer that want to join project: Fix code style warnings :)
##########
File path: pom.xml
##########
@@ -1084,6 +1122,22 @@
</reportSet>
</reportSets>
</plugin>
+ <!-- plugin>
Review comment:
The reporting section I'm not sure if those inline configs work.
The other thing about shared config file rather than inline configs is that the checkstyle.xml can be used in your favourite editor. So you get those warnings while coding. Not while running maven.
##########
File path: pom.xml
##########
@@ -804,6 +805,43 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <version>${maven-checkstyle-plugin.version}</version>
+ <configuration>
+ <checkstyleRules>
+ <module name = "Checker">
+ <property name="fileExtensions" value="java,js,css,xml"/>
Review comment:
I think you enabled the file extensions successfully.
But TreeWalker only works for Java file types. https://checkstyle.sourceforge.io/config.html#TreeWalker
> FileSetCheck TreeWalker checks individual **Java source** files and defines properties that are applicable to checking such files.
I think js you can forget, but the google reference checkstyle config checks additionally XML and properties files.
https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml
And those rules outside of TreeWalker in above google XML should also work for non Java files. Except they have additional rules.
But yeah its really Java centric. For JS I think you should have a linter filer and invoke a eslint. You can just run Eslint from command line. Or invoke it via a maven wrapper that invokes a command line.
And it will be much better (and more easy) to re-use eslint files from other projects for JavaScript checks. And prob also more what people expect that code Javascript.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on a change in pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#discussion_r415515287
##########
File path: pom.xml
##########
@@ -804,6 +805,43 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-checkstyle-plugin</artifactId>
+ <version>${maven-checkstyle-plugin.version}</version>
+ <configuration>
+ <checkstyleRules>
+ <module name = "Checker">
+ <property name="fileExtensions" value="java,js,css,xml"/>
Review comment:
@sebawagner `fileExtensions` were added - but it doesn't work ...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #69: OPENMEETINGS-2307 Add checkstyle check with only checking for unused …
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#discussion_r415413929
##########
File path: src/config/openmeetings_checkstyle.xml
##########
@@ -0,0 +1,33 @@
+<?xml version="1.0"?>
+<!--
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<!DOCTYPE module PUBLIC
+ "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
+ "https://checkstyle.org/dtds/configuration_1_3.dtd">
+
+<!--
+ Checkstyle configuration that checks OpenMeetings Java Source files.
+ We are not checking much yet, but rather one by one.
+ Authors: Sebastian Wagner
+ -->
+
+<module name = "Checker">
+
+ <property name="fileExtensions" value="java" />
+
+ <module name="TreeWalker" >
+ <module name="UnusedImports"></module>
Review comment:
Add you checks here if you want more.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619874905
@sebawagner "Trailing space" checker is outside `TreeWalker` and it doesn't work with files other than `*.java`
One of the most weird plugin I ever use ....
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #69: OPENMEETINGS-2307 Add checkstyle check with simplistic checks
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #69:
URL: https://github.com/apache/openmeetings/pull/69#issuecomment-619912195
On Mon, 27 Apr 2020 at 18:08, Sebastian Wagner <no...@github.com>
wrote:
> I know but still I don't think there is any real alternative in the Java
> world.
>
> It's also quite a difficult task: Write a code that understand a code
> language and is configurable AND do that across different languages.
>
> Checkstyles doesn't just apply regexp it reads the code and tries to
> understand the syntax and logic.
>
IMO if something has name Regex - it should work as regex :)
It doesn't work
Or Im unable to configure it :)))
>
> It's just never been intended to use it for with other then Java. But for
> Java it's really good.
>
> Thx
> Seb
>
> On Mon, 27 Apr 2020, 10:01 PM Maxim Solodovnik, <no...@github.com>
> wrote:
>
> > @sebawagner <https://github.com/sebawagner> "Trailing space" checker is
> > outside TreeWalker and it doesn't work with files other than *.java
> > One of the most weird plugin I ever use ....
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <https://github.com/apache/openmeetings/pull/69#issuecomment-619874905>,
> > or unsubscribe
> > <
> https://github.com/notifications/unsubscribe-auth/ABHJ2QD3SXF3EHKFDMEOUP3ROVJY5ANCNFSM4MRNXWXQ
> >
> > .
> >
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/openmeetings/pull/69#issuecomment-619910830>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AA5Q673WW7T6G5AXMIUHAZ3ROVRR5ANCNFSM4MRNXWXQ>
> .
>
--
Best regards,
Maxim
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org