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

#307: change offer ids to be represented in requests and response as long data type #386

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

sreuland
Copy link
Contributor

@sreuland sreuland commented Dec 3, 2021

Problem: some offer id values were modeled as string and others as integral longs in request and paired response objects.

Fix: make the offer id consistently modeled as integral long type in request and response objects.

Note: to achieve the consistency, this introduces breaking changes on existing public classes - changed some existing method signatures for offer ids on request and response models to use long data type instead of string, details in CHANGELOG. Looking for feedback on the approach, if it's worth it or not.

Closes #307

@sreuland sreuland self-assigned this Dec 3, 2021
Copy link
Contributor

@paulbellamy paulbellamy left a comment

Choose a reason for hiding this comment

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

Looks good to me. Discussed in person whether we want to go with a Long, or a String. In the go sdk we use String, but let's look for other precedent in here and other sdks to match.

Edit: And for the breaking change, let's do a minor release. It is type-change only so should fail to compile if the user updates.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@tamirms tamirms left a comment

Choose a reason for hiding this comment

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

I agree that the offer id should be long the reason they are strings in the javascript sdk is that the JSON does not support 64 bit integers but the java JSON parser does not have that problem

@sreuland sreuland merged commit 94437f5 into lightsail-network:master Dec 8, 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.

offer_id in revoke_sponsorship
3 participants