-
Notifications
You must be signed in to change notification settings - Fork 150
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
♻️ ⏪ Revert changes for old queries #3579
♻️ ⏪ Revert changes for old queries #3579
Conversation
f12184e
to
14195ae
Compare
14195ae
to
24a7e67
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
- (Libplanet.Explorer) Added `LegacyBencodexValueType` class that is | ||
a copy of an old `BencodexValueType` with its name changed | ||
for backwards compatibility. Changed old `states` query | ||
to use `LegacyBencodexValueType` instead of `BencodexValueType`. [[#3579]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add issue or PR numbers about previous feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? I guess we can refer to #3560 maybe, but I don't think it'd help much. Also the whole point of this PR is to not fix the issue since we don't want to deal with the consequences of breaking the API. 😑
|
||
namespace Libplanet.Explorer.GraphTypes | ||
{ | ||
public class LegacyBencodexValueType : StringGraphType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does we haven't more good naming...? I guess if you have plan about more refactoring, maybe you need LegacyBencodexValueV2Type later. 🙄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's just more work then. To be more sensible and be managable, we need an entirely new set of "V2" scheme which doesn't share the root. Then we'd have to create an entirely new copies of query related codes, management of V1 and V2 graph types, etc. 🙄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5b49b75
into
planetarium:3.9-maintenance
This should resolve the backwards compatibility issue with services already using the
states
query.It needs to be phased out anyway, so might as well keep it. 🙄
P.S. From what I gather,
LegacyBencodexValueType
was implemented wrongly in the first place (serialization and parse mismatch, implicit base64 representation in playground, etc.). Hence, resulting in such an awkward way to force backwards compatibility in this PR. 😐