Fix: Downgrade reflections to 0.9.11 (fixes #60) #61
Conversation
porunov
force-pushed
the
rollback-reflections-to-0.9.11
branch
from
a750fe0
to
edd69cf
Compare
|
@porunov ahh wonderful, includes a unit test. perfect. I'll review tonight most likely maybe tomorrow (11:30pm here). Glad to see you added yourself as a contributor as well and the changelog, looks good at first glance just need to test it locally. |
|
@freemo Thank you! The unit test should fail with reflections |
Perfect, thanks. By the way I can and will approve pull requests over here at github, but it requires extra steps from me (I need to check it out and recreate the PR over on gitlab). If you plan to be a regular contributor it would really help if you could submit the pull requests on our gitlab instance instead. However if that will be a barrier that prevents you from contributing feel free to continue to contribute them here. |
|
@freemo Sure. I have created a PR in GitLab here: https://git.qoto.org/Ferma/Ferma/-/merge_requests/1 It is my first usage of GitLab, so if I do something wrong, just tell me |
|
I think its good, thanks. I'll review in more detail shortly.
…On Sat, Mar 7, 2020 at 1:50 AM Oleksandr Porunov ***@***.***> wrote:
@freemo <https://github.com/freemo> Sure. I have created a PR in GitLab
here: https://git.qoto.org/Ferma/Ferma/-/merge_requests/1
It is my first usage of GitLab, so if I do something wrong, just tell me
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#61?email_source=notifications&email_token=AAXESARJD6N2H3YYNIFRRLTRGGK4DA5CNFSM4LDJBHW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEODJOAQ#issuecomment-596023042>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXESAVNDIJOK3J5ONR3W53RGGK4DANCNFSM4LDJBHWQ>
.
|
|
@freemo I was also researching other options yesterday. I noticed the next reflections fork: What do you think about it? |
|
That sounds like an OK solution to me. honestly its a temporary solution
and i dont think will be very critical either way so long as it works. IF
that gives the problem it looks like a version bump so what could be the
problem?
…On Sat, Mar 7, 2020 at 9:07 PM Oleksandr Porunov ***@***.***> wrote:
@freemo <https://github.com/freemo> I was also researching other options
yesterday. I noticed the next reflections fork:
https://github.com/aschoerk/reflections8
Current latest version is 0.11.7:
https://search.maven.org/search?q=net.oneandone.reflections8
It says: Since org.reflections 0.9.12 is released: obsolete.
But the thing is that 0.11.7 version of reflections8 is newer then 0.9.11
of reflections. I noticed that they don't have such bug (didn't test it
yet but the code of Reflections.class looks fine to me).
Similar to 0.9.12 is no longer depends on guava but depends on java 8.
So, I was thinking, we might give a try for 0.11.7 of reflections8 and
after 0.9.13 of reflections is released, switch back to reflections
(assuming it fixes the bug).
What do you think about it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#61?email_source=notifications&email_token=AAXESAVUMTW2XPRLIVT3VXTRGKSPZA5CNFSM4LDJBHW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOEEIOA#issuecomment-596132920>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXESASYVU4ICSVCYCGQPW3RGKSPZANCNFSM4LDJBHWQ>
.
|
|
@freemo Overall it isn't a big problem because we can exclude transitive dependencies but it would be good to exclude guava from dependencies because it often becomes a problem when you have several libraries which both compiled with using different guava versions and that is why they might be not compatible because guava versions are often incompatible with older guava versions. |
|
I think we directly depend on Guava. If we remove it from our dependencies
then it would break Ferma except when Guava is explicitly included in
addition to it. Which I dont think is a good solution.
By the way im pulling down your PRs now and reviewing them. Will have the
bulk of them in shortly.
…On Sat, Mar 7, 2020 at 11:32 PM Oleksandr Porunov ***@***.***> wrote:
@freemo <https://github.com/freemo> Overall it isn't a big problem
because we can exclude transitive dependencies but it would be good to
exclude guava from dependencies because it often becomes a problem when you
have several libraries which both compiled with using different guava
versions and that is why they might be not compatible because guava
versions are often incompatible with older guava versions.
I checked 0.11.7 of reflections8 it works as expected and doesn't
introduce guava dependency (same as 0.9.12 of reflections). I will check
how much Ferma depends on guava, maybe we can move to use Java 8 instead of
guava. If so, I will provide the PR shortly
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#61?email_source=notifications&email_token=AAXESAVGYHOUP5D2IBBHOYTRGLDPLA5CNFSM4LDJBHW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOEG7NI#issuecomment-596144053>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXESAR74LQM4ZFS4CEUEADRGLDPLANCNFSM4LDJBHWQ>
.
|
|
@freemo I know that Ferma directly depends on guava but are you OK if we move all guava implementations to java 8 implementations (i.e. streams API and direct java utils usage)? I think it isn't hard to detach Ferma from guava. After that we can just remove guava dependency from Ferma. Or do you have another point of view regarding the guava (i.e. like cleaner code at some places or something like that)? |
|
I am just asking you, because I have noticed that many libraries (datastax cassandra driver, reflections, etc.) are moving out from guava due to dependency problems. It is much easier to include dependency which doesn't depend on guava then dependency which depends on guava due to binary incompatibility between many guava versions. |
|
I am perfectly ok moving it all to java8 yes. As long as there are no
unexpected issues that arise from that that I cant foresee its good by me.
…On Sun, Mar 8, 2020 at 12:05 AM Oleksandr Porunov ***@***.***> wrote:
@freemo <https://github.com/freemo> I know that Ferma directly depends on
guava but are you OK if we move all guava implementations to java 8
implementations (i.e. streams API and direct java utils usage)? I think it
isn't hard to detach Ferma from guava. After that we can just remove guava
dependency from Ferma. Or do you have another point of view regarding the
guava (i.e. like cleaner code at some places or something like that)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#61?email_source=notifications&email_token=AAXESAVJSJFGXUAHLXZE6L3RGLHM5A5CNFSM4LDJBHW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOEHSII#issuecomment-596146465>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXESASW75DYBWWSHVW7XQLRGLHM5ANCNFSM4LDJBHWQ>
.
|
|
Yea at the time that I wrote Ferma things were different, java8
didnt exist. I completely support the move.
…On Sun, Mar 8, 2020 at 12:08 AM Oleksandr Porunov ***@***.***> wrote:
I am just asking you, because I have noticed that many libraries (datastax
cassandra driver, reflections, etc.) are moving out from guava due to
dependency problems. It is much easier to include dependency which doesn't
depend on guava then dependency which depends on guava due to binary
incompatibility between many guava versions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#61?email_source=notifications&email_token=AAXESATMUVHBRSISL6FO66TRGLHXVA5CNFSM4LDJBHW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOEHT2I#issuecomment-596146665>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXESARHQQM5XHLXKZOD6ALRGLHXVANCNFSM4LDJBHWQ>
.
|
|
Suppressed by #63 |
Fixes #60