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

upgrade squid bridge #158

Closed
wants to merge 3 commits into from
Closed

upgrade squid bridge #158

wants to merge 3 commits into from

Conversation

hypery2k
Copy link

Issue #157 upgrade squid bridge

<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>15.0</version>
Copy link
Member

@rnveach rnveach Sep 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause issues with checkstyle as it uses guava 26? Why do we need this dependency if no code was changed?

@rnveach
Copy link
Member

rnveach commented Sep 16, 2018

@hypery2k CI must be green.

File 'pom.xml' uses tabs for indentation

@romani wercker error:

[ERROR] SonarQube server [http://localhost:9000] can not be reached

@hypery2k
Copy link
Author

did you have a formatter for IDEA?

@rnveach
Copy link
Member

rnveach commented Sep 16, 2018

@hypery2k I'm not sure I understand your question. It isn't a formatter, it is just asking to use spaces instead of tabs for indentation. This should be a simple change using notepad or such. Just follow formatting of similar items around it.

@hypery2k
Copy link
Author

I cannot get the CI build green, as either team city or checkstyle will complain about this. And I didn't changed anything there...

@rnveach
Copy link
Member

rnveach commented Sep 17, 2018

weird it was only complaining about the indentation before. It looks like it is related to the new TC version.

@aepfli
Copy link

aepfli commented Sep 24, 2018

is it possible to support somehow? i am not sure how to fix it, but at least i understand the problem in CheckstyleExecutor with "Call to 'Collection.toArray()' with zero-length array argument (1)"

return new URLClassLoader(urls.toArray(new URL[urls.size()]), null);

but a good suggestion based on h2database/h2database#311 would be to change the line simply to return new URLClassLoader(urls.toArray(new URL[0]), null);

@rnveach
Copy link
Member

rnveach commented Sep 24, 2018

would be to change the line simply to return new URLClassLoader(urls.toArray(new URL[0]), null);

Yes, this is the change that would have to be made.

@hypery2k
Copy link
Author

with the proposed changes I then get maven pmd errors:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-pmd-plugin:3.7:check (default) on project checkstyle-sonar-plugin: You have 1 PMD violation. For more details see: /Users/mreinhardt/Documents/Programmierung/repositories/git/job/sonar-checkstyle/checkstyle-sonar-plugin/target/pmd.xml -> [Help 1]

@rnveach
Copy link
Member

rnveach commented Sep 24, 2018

@hypery2k You would have to show us the PMD error as that is all the error shows. Most likely it isn't related to your PR but TC changed between the last master and now.

@hypery2k
Copy link
Author

see here:

<?xml version="1.0" encoding="UTF-8"?>
<pmd version="5.5.1" timestamp="2018-09-24T16:27:44.057">
<file name="/Users/mreinhardt/Documents/Programmierung/repositories/git/job/sonar-checkstyle/checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleExecutor.java">
<violation beginline="146" endline="146" begincolumn="35" endcolumn="58" rule="OptimizableToArrayCall" ruleset="Design" package="org.sonar.plugins.checkstyle" class="CheckstyleExecutor" method="createClassloader" externalInfoUrl="https://pmd.github.io/pmd-5.5.1/pmd-java/rules/java/design.html#OptimizableToArrayCall" priority="3">
This call to Collection.toArray() may be optimizable
</violation>
</file>
</pmd>

@aepfli
Copy link

aepfli commented Sep 24, 2018

Well I do think that this pmd rule, based on the link I posted is not 100% valid ;) - I am not a contributor to sonar-checkstyle, but can we deactivate this rule?

@rnveach
Copy link
Member

rnveach commented Sep 26, 2018

Main repo uses the same IDEA and PMD doesn't complain there, so I am trying to reconcile the 2. See #159 . Sorry for delay.

@romani
Copy link
Member

romani commented Oct 4, 2018

@hypery2k , I apologize for delay in response.

Please squash all commits in one and rebase latest master (we recently merged some fixes for CI), and will continue investigate what else is required to make this PR be merged.

Please also make some comments in issue about what need to be changed in code to let us validate a fix. We need help from you.

@rnveach
Copy link
Member

rnveach commented Nov 9, 2018

@hypery2k ping

@romani
Copy link
Member

romani commented Dec 8, 2018

closed in favor of #165

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.

None yet

4 participants