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

Change API field naming style to Camel Case. #164

Closed

Conversation

overcat
Copy link
Contributor

@overcat overcat commented May 9, 2024

What

Change API field naming style to Camel Case.

Why

Other APIs all use Camel Case style, let's keep it consistent.

Known limitations

[TODO or N/A]

@@ -131,8 +131,8 @@ func (l *LedgerEntryChange) FromXDRDiff(diff preflight.XDRDiff) error {
return nil
}

// LedgerEntryChange designates a change in a ledger entry. Before and After cannot be be omitted at the same time.
// If Before is omitted, it constitutes a creation, if After is omitted, it constitutes a delation.
// LedgerEntryChange designates a change in a ledger entry. Before and After cannot be omitted at the same time.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct the typo here by the way.

@Shaptic
Copy link
Contributor

Shaptic commented May 9, 2024

Fyi @psheth9 after #132, PTAL!

Also this is not technically a breaking change because we haven't released/deployed RPC since #132 went in adding this endpoint 🎉

@psheth9
Copy link
Contributor

psheth9 commented May 14, 2024

Sadly, this is now a breaking change because we already released getVersionInfo endpoint https://github.com/stellar/soroban-rpc/releases/tag/v21.1.0 -- so now this change should go in next protocol bump just fyi

Copy link
Contributor

@psheth9 psheth9 left a comment

Choose a reason for hiding this comment

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

LGTM !!

@psheth9 psheth9 added the breaking-change Breaking change tag label May 15, 2024
@2opremio
Copy link
Contributor

@Shaptic are there downstream tickets to fix this?

@2opremio
Copy link
Contributor

@janewang When would it be a good time to release this? (considering it's a breaking change)

@Shaptic
Copy link
Contributor

Shaptic commented Jun 13, 2024

Not yet, since the answer is dependent on the release cycle (unfortunately I think we'd have to wait until Protocol 22 as it means breaking releases for all of the SDKs).

@Shaptic Shaptic changed the base branch from main to v22-breaking-changes July 17, 2024 16:15
@janewang
Copy link
Collaborator

+1, Protocol 22 is the time

@2opremio
Copy link
Contributor

2opremio commented Aug 29, 2024

I resolved the conflicts and cherry-picked onto #280

Thanks @overcat !

@2opremio 2opremio closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaking change tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants