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

Remove the dependency on Guava. #524

Merged
merged 24 commits into from
Sep 18, 2023

Conversation

overcat
Copy link
Member

@overcat overcat commented Sep 11, 2023

Part of #496

This PR does not include XDR changes, I need to wait for the merge of stellar/xdrgen#168 before initiating a separate XDR-related PR.


This PR is almost completed, but before merging it, we need to merge #525 and #527 first and remove com.google.guava:guava:32.1.2-android in build.gradle.

@overcat
Copy link
Member Author

overcat commented Sep 11, 2023

Currently, Java 8 does not have built-in support for Base16 and Base32. Are you inclined to introduce the Apache Commons Codec package or implement it ourselves in the SDK?

Update:
I have added commons-codec to the dependencies list.

@overcat overcat changed the title [WIP] Remove the dependency on Guava. Remove the dependency on Guava. Sep 12, 2023
@overcat overcat marked this pull request as ready for review September 12, 2023 10:22
@overcat overcat changed the title Remove the dependency on Guava. [DONT MERGE] Remove the dependency on Guava. Sep 12, 2023
@sreuland
Copy link
Contributor

Currently, Java 8 does not have built-in support for Base16 and Base32. Are you inclined to introduce the Apache Commons Codec package or implement it ourselves in the SDK?

Update: I have added commons-codec to the dependencies list.

ok, if interested to avoid the additional dep on commons-codec, can get by with creating a helper method in plain java using String.format - https://www.baeldung.com/java-hexformat#dealing-with-hex-strings-before-java-17

@overcat overcat changed the title [DONT MERGE] Remove the dependency on Guava. Remove the dependency on Guava. Sep 12, 2023
@overcat
Copy link
Member Author

overcat commented Sep 12, 2023

This PR is almost completed. After checking it again tomorrow, I will request a review :-)

@overcat
Copy link
Member Author

overcat commented Sep 13, 2023

commons-codec looks relatively simple and doesn't have a series of dependencies like Guava, so I think it would be great to introduce it, right?

@overcat
Copy link
Member Author

overcat commented Sep 13, 2023

Hi @sreuland, this PR is ready for review, it's big PR 😂

@sreuland
Copy link
Contributor

sreuland commented Sep 18, 2023

@overcat , #530 was merged into soroban, looks like need to port latest soroban back to here to finalize. I updated pr with port from latest on soroban - 6c28a2e, tests passing afterwards, looks ready to merge.

@sreuland sreuland merged commit 302aba5 into lightsail-network:soroban Sep 18, 2023
7 checks passed
@overcat
Copy link
Member Author

overcat commented Sep 19, 2023

@overcat , #530 was merged into soroban, looks like need to port latest soroban back to here to finalize. I updated pr with port from latest on soroban - 6c28a2e, tests passing afterwards, looks ready to merge.

Great, thanks @sreuland.

@overcat
Copy link
Member Author

overcat commented Sep 19, 2023

Hi @sreuland, It seems that the conflict has not been resolved correctly. Please check https://github.com/stellar/java-stellar-sdk/pull/524/files#diff-04e963815abf17aa3afc8545dd7431ba59aa861e57e3a04f53c5c33b82482305L12

Do you mind if I revert this PR and then create a new PR to merge into the soroban branch, and finally squash these three commits on the soroban branch?

overcat added a commit that referenced this pull request Sep 19, 2023
overcat added a commit to overcat-deprecation/java-stellar-sdk that referenced this pull request Sep 19, 2023
overcat added a commit that referenced this pull request Sep 19, 2023
overcat added a commit that referenced this pull request Sep 19, 2023
Revert "Remove the dependency on Guava. (#524)" (#533)

This reverts commit 302aba5.

Remove the dependency on Guava. (#524)
@overcat
Copy link
Member Author

overcat commented Sep 19, 2023

I have already fixed this issue in the soroban branch and pushed the code after squashing by using git push origin soroban --force-with-lease

@overcat overcat deleted the remove-guava branch September 19, 2023 03:31
@sreuland
Copy link
Contributor

ok, I had done manual merge of conflicts in SimulateTransactionResponse and SorabanServerTest after porting soroban and looks like chose incorrect direction, thanks for resolving.

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.

2 participants