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

Added back the old logic for classpath elements #51

Merged
merged 1 commit into from
Jun 16, 2016

Conversation

amihaiemil
Copy link
Member

@amihaiemil amihaiemil commented May 17, 2016

This is for issue #48 .
Changes in commit 4411738 fixed issue #41 , however they broke the classpath elements look-up logic. With those changes, only the elements in the compile classpath are searched. (E.g. the example project (findbugs-annotations) didn't work because jar aspectjrt was runtime scoped. It worked fine when I made the jar compile scoped)

Fix Let the changes from commit 4411738 and also added back the old lookup logic (now they are combined, method classpath() returns both the artifacts found by our logic and those found by ${project.compileClasspathElements}.
Also, classpath() method now returns a collection of String filepaths, instead of Files, it's a bit lighter.

I tested and now it works for both the example projects (1 attached in issue #48 - findbugs-annotations and 1 attached in issue #41).

@amihaiemil
Copy link
Member Author

@dmarkov This PR is for issue #48 .

@dmarkov
Copy link

dmarkov commented May 18, 2016

@amihaiemil Thanks, someone will review your pull request soon

@dmarkov
Copy link

dmarkov commented May 18, 2016

@dmzaytsev could you please review this pull request

@amihaiemil
Copy link
Member Author

@dmzaytsev ping

@dmzaytsev
Copy link

@dmarkov please assign someone else
no time

@dmarkov
Copy link

dmarkov commented May 19, 2016

@dmarkov please assign someone else
no time

@dmzaytsev -15 points to your rating :(

@dmarkov
Copy link

dmarkov commented May 19, 2016

@dmarkov please assign someone else
no time

@dmzaytsev I will assign somebody else to this issue

@amihaiemil
Copy link
Member Author

@krzyk Can you maybe ask the PM to assign this to you so we can move faster? : )

@krzyk
Copy link

krzyk commented May 19, 2016

@amihaiemil it is up to @yegor256 he is the architect here

@dmarkov
Copy link

dmarkov commented May 19, 2016

@mkordas review this one please

@mkordas
Copy link

mkordas commented May 19, 2016

@amihaiemil I'm on it

@mkordas
Copy link

mkordas commented May 20, 2016

@amihaiemil sorry, somehow I've lost focus yesterday, back here again

@amihaiemil
Copy link
Member Author

@mkordas no worries, haha : D

@mkordas
Copy link

mkordas commented May 20, 2016

@amihaiemil I need explanation on

Fix Let the changes from commit 4411738 and also added back the old lookup logic (now they are combined, method classpath() returns both the artifacts found by our logic and those found by ${project.compileClasspathElements}.

What is exactly done besides revert and changing return type to Collection<String>. You mention compileClasspathElement but I couldn’t find it.

@mkordas
Copy link

mkordas commented May 20, 2016

@amihaiemil see just one question

@amihaiemil
Copy link
Member Author

@mkordas see this.classpathElements. That was added in that commit. And it is combined with the old logic in the last line of classpath(). elements.addAll(this.classpathElements).

@mkordas
Copy link

mkordas commented May 21, 2016

@amihaiemil thanks, looks good!

@mkordas
Copy link

mkordas commented May 21, 2016

@rultor merge

@rultor
Copy link
Contributor

rultor commented May 21, 2016

@rultor merge

@mkordas Thanks for your request. @yegor256 Please confirm this.

@amihaiemil
Copy link
Member Author

@yegor256 Can you please see about this one? Thanks

@amihaiemil
Copy link
Member Author

@yegor256 ping

@amihaiemil
Copy link
Member Author

@yegor256 don't forget about this one, pls. Thanks

@amihaiemil
Copy link
Member Author

@yegor256 ping

@mkordas
Copy link

mkordas commented May 28, 2016

@yegor256 please review this PR

@mkordas
Copy link

mkordas commented May 29, 2016

@yegor256 ping

@mkordas
Copy link

mkordas commented May 30, 2016

@yegor256 ping again

@amihaiemil
Copy link
Member Author

@yegor256 ping

1 similar comment
@mkordas
Copy link

mkordas commented Jun 2, 2016

@yegor256 ping

@mkordas
Copy link

mkordas commented Jun 3, 2016

@yegor256 ping one more time

@mkordas
Copy link

mkordas commented Jun 4, 2016

@yegor256 ping

@mkordas
Copy link

mkordas commented Jun 5, 2016

@yegor256 it's waiting for you 15 days already

@mkordas
Copy link

mkordas commented Jun 6, 2016

@yegor256 ping

@mkordas
Copy link

mkordas commented Jun 7, 2016

@yegor256 ping again

@amihaiemil
Copy link
Member Author

@mkordas maybe we can write a script that pings every 24 hours, eh? :))

@mkordas
Copy link

mkordas commented Jun 7, 2016

@amihaiemil yeah, I'm not sure what else can we do here...

@mkordas
Copy link

mkordas commented Jun 8, 2016

@yegor256 please take a look at this PR

@mkordas
Copy link

mkordas commented Jun 9, 2016

@yegor256 ping

@mkordas
Copy link

mkordas commented Jun 10, 2016

@yegor256 ping again

@mkordas
Copy link

mkordas commented Jun 12, 2016

@yegor256 ping

@krzyk
Copy link

krzyk commented Jun 14, 2016

@yegor256 could we get this merged? This issue has been opened for quite long and it makes findbugs practically disabled in projects using few latest qulice releases.

@yegor256
Copy link
Member

@rultor merge

@rultor
Copy link
Contributor

rultor commented Jun 16, 2016

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 45d7d52 into jcabi:master Jun 16, 2016
@rultor
Copy link
Contributor

rultor commented Jun 16, 2016

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 9min)

@dmarkov
Copy link

dmarkov commented Jun 17, 2016

@mkordas Thanks a lot, I just topped your account for 35 mins, transaction ID 5763c883b93e110349000bdc, total time was 670 hours and 1 min... extra minutes for review comments (c=20)... +35 to your rating, your total score is +5488

@dmarkov
Copy link

dmarkov commented Jun 17, 2016

@rultor deploy pls

@rultor
Copy link
Contributor

rultor commented Jun 17, 2016

@rultor deploy pls

@dmarkov OK, I'll try to deploy now. You can check the progress here

@rultor
Copy link
Contributor

rultor commented Jun 17, 2016

@rultor deploy pls

@dmarkov Done! FYI, the full log is here (took me 7min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants