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

Make constructures public to allow creating objects without gson. #390

Merged
merged 1 commit into from
Dec 20, 2021
Merged

Conversation

lijamie98
Copy link
Contributor

No description provided.

@lijamie98 lijamie98 requested a review from tamirms December 17, 2021 17:43
@@ -393,7 +393,7 @@ public String get(String key) {
@SerializedName("transactions")
private final Link transactions;

Links(Link effects, Link offers, Link operations, Link self, Link transactions) {
public Links(Link effects, Link offers, Link operations, Link self, Link transactions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@lijamie98 , was just curious what is the use case driving the need to populate the model manually rather than from server api responses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@lijamie98 lijamie98 Dec 17, 2021

Choose a reason for hiding this comment

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

It is actually the Signer class that the tests need a public constructor. However, similar pattern may occur when we do tests in the future.

Builder pattern would work too if public constructors are not preferred. Changing to Builder pattern will also need to change the way we construct AccountResponse.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it's for testing concerns. I noticed a few other response POJO's here in the SDK already had public constructors on their inner classes, so, hopefully this is contained to AccountResponse. I wanted to also mention, Spring's ReflectionTestUtils , as an optional brute force way for tests to access private members of classes under test. I didn't see Spring in the anchor project's pom.xml, but maybe could use that with test scope if this issue of access crops up more.

Yes, It might be nice to for us to consider adding Lombok here into the SDK on these POJOs, and bring in builders. I see you have that in the anchor project as well, nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the anchor dev project requirements is NOT to use Spring :-(. So, I used gson to bruteforce it. However, I think this can be better solved with either a builder or public constructor.

Lombok is a real nice way to reduce the verboseness of Java. :-) +1

I will change the CHANGELOG on Monday and update the PR.

@sreuland
Copy link
Contributor

@lijamie98 , would you mind adding a line here in CHANGELOG to note modification? since it's client facing, thanks!

@lijamie98
Copy link
Contributor Author

lijamie98 commented Dec 17, 2021 via email

@lijamie98
Copy link
Contributor Author

CHANGELOG.md is also updated.

@sreuland
Copy link
Contributor

@lijamie98 , looks like the commit from your github acct doesn't have gpg signature enabled, the PR can't metge non signed commits in this repo, can you redo commit after gpg enabled? Or lmk, i can assist.

@lijamie98
Copy link
Contributor Author

Thanks for offering the assitance! The commit was signed but the email that I used to sign the PR was mismatching.

Fixed the user.email config in my git. It is now work.

@lijamie98 lijamie98 merged commit aad2ca2 into lightsail-network:master Dec 20, 2021
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.

3 participants