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

api: Invert StartDealParams fast retrieval param #3026

Closed
wants to merge 1 commit into from

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Aug 13, 2020

This should set better default expectations

Fixes #3017

@magik6k magik6k requested review from momack2 and arajasek August 13, 2020 09:06
@momack2
Copy link
Contributor

momack2 commented Aug 13, 2020

Isn't different experiences between the API and CLI still confusing? Seems like we should align them to all say the same thing and have the same default value. With this change 'setting the fast retrieval flag to true' could actually mean opposite things. Is it hard to make the default value true here? that would be my preference...

@ribasushi
Copy link
Collaborator

@momack2 the way the RPC pipeline is built, there is no way to signal "not set" - there's just Set=true and Set=false / unset without an ability to tell the two sub-states apart

@magik6k "fast/slow-retrieval" is too far removed from what actually happens anyway. If we are to rename things we should probably harmonize everything towards RemoveUnsealedSource: => false(default) / true

@momack2
Copy link
Contributor

momack2 commented Aug 13, 2020

From @whyrusleeping "If we really want to make this change, i think adding a JSONUnmarshaler method to the struct in question that defaults the value to true seems more reasonable" I would be happy with that change.

Anything involving renaming across the cli/api seems more involved and complex and I'd encourage us deferring because the changes across the existing code (and the messaging) would be much more difficult to wrangle. If adding the unmarshaler isn't an option, let's just close my request and document it better.

@magik6k
Copy link
Contributor Author

magik6k commented Aug 17, 2020

JSONUnmarshaler sounds better, closing this PR for now

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