Skip to content

PascalCase enum members in REST API responses#2323

Merged
achamayou merged 15 commits into
microsoft:mainfrom
letmaik:letmaik/PascalCase4JsonEnums
Mar 16, 2021
Merged

PascalCase enum members in REST API responses#2323
achamayou merged 15 commits into
microsoft:mainfrom
letmaik:letmaik/PascalCase4JsonEnums

Conversation

@letmaik
Copy link
Copy Markdown
Member

@letmaik letmaik commented Mar 15, 2021

Fixes #2152.

@letmaik letmaik requested a review from a team as a code owner March 15, 2021 18:23
{ccf::State::readingPublicLedger, "readingPublicLedger"},
{ccf::State::readingPrivateLedger, "readingPrivateLedger"},
{ccf::State::verifyingSnapshot, "verifyingSnapshot"}})
{{ccf::State::uninitialized, "Uninitialized"},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be ccf::State::UNINITIALIZED, "Uninitialized"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not changing any C++ enum names in this PR, just serialization. But yes, this should eventually be changed.

Comment thread src/node/rpc/tx_status.h
@letmaik letmaik marked this pull request as draft March 15, 2021 18:54
Comment thread CHANGELOG.md Outdated

### Changed

- Enum values returned by built-in REST API endpoints are now PascalCase (#2152).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The public Python API has been updated to use all-caps enums, that should be mentioned too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's actually the same, only the values are updated, but the enum names are the same.

@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2021

letmaik/PascalCase4JsonEnums@20765 aka 20210316.42 vs main ewma over 20 builds from 20233 to 20742
images

PASSED = 1
PENDING = 0
REJECTED = -1
STATE_ACTIVE = "ACTIVE"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewers, take note!

@letmaik letmaik marked this pull request as ready for review March 16, 2021 11:35
Comment thread tests/infra/network.py Outdated
Comment thread tests/infra/proposal.py
Comment on lines +21 to +25
OPEN = "Open"
ACCEPTED = "Accepted"
WITHDRAWN = "Withdrawn"
REJECTED = "Rejected"
FAILED = "Failed"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

None of the other Python enum names have changed, only this one. If we're not touching the code names, only the values, the left hands here should be reverted.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hm, I didn't want to change C++ enums but Python test code seems ok? Is there anything that could break?

Copy link
Copy Markdown
Member

@eddyashton eddyashton Mar 16, 2021

Choose a reason for hiding this comment

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

Its harmless, just seems odd to do a partial consistency fix so I assumed it was a mistake.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I fixed the ones that are in sync with C++. There are some local ones which I didn't touch.

@achamayou achamayou merged commit 674c4f1 into microsoft:main Mar 16, 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.

PascalCase enum members for endpoint responses

4 participants