Add support for range voting e.g. "70802-9" #136
Conversation
Signed-off-by: John Plevyak <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
src/commands/neuron_manage.rs
Outdated
| } | ||
| if proposals.is_empty() { | ||
| return Err(anyhow!( | ||
| "Proposal ranges must be less than 100 and in the form XXX-YY." |
There was a problem hiding this comment.
This message is confusing at best: it claims the format HAS to be XXX-YY, but if the XXX part was as strictly enforced as the YY part it wouldn't allow me to specify any proposals larger than 999.
There was a problem hiding this comment.
Suggestions? Perhaps just X-Y ?
There was a problem hiding this comment.
Let's start with <a number>-<a number greater than the first number> as valid. So these are ok:
- 465-475
- 11033-11270
We can consider forms of shorthand once those are supported, but I think we should be sure that it's worth it / likely to be used. While one might see shorthand like "349-51" on a piece of paper, I'm not sure how many would expect that to be valid input for a command line.
There was a problem hiding this comment.
I use 78825-8 for example all the time. We often have small ranges in sets of upgrade or replace proposals. I think that X-Y encourages the natural interpretation which is the unambiguous suffix semantics e.g. 123-5 123-25 123-125 are all the same and 123-1 is the empty range i.e. there is no natural interpretation that it might be say 131 based on some notion carry. For documentation like this I think less is more sometimes. If someone doesn't use the additional suffix capability it isn't the worst thing.
ericswanson-dfinity
dismissed
their stale review
Dismissing because I am going on PTO.
There was a problem hiding this comment.
I've dismissed my review requesting changes, only because I'm going on PTO.
There are two questions to be answered here:
- Is the proposed interface acceptable from a UX perspective?
- Is the implementation clear, and free of defects?
For (1), I would answer no. There are two reviewers offering the opinion that the interface is not intuitive. It would be nice if the submitter would take that into consideration. Can the submitter point to another command-line interface that provides such an interface? We shouldn't add a special-case interface that only the submitter of this PR will use.
For (2), I would also answer no. As written, in my opinion the supported UX is not at all clear from a reading of the code. At minimum the range selection code should be refactored into a separate function, with unit tests, but beyond that, it's not obvious what is going on with the replace_range and resize_with calls.
message. Signed-off-by: John Plevyak <[email protected]>
Co-authored-by: Severin Siffert <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
This reverts commit 605ddbf.
* Add support for range voting e.g. "70802-9". Signed-off-by: John Plevyak <[email protected]> Co-authored-by: Severin Siffert <[email protected]>
Description
Add support for proposal ranges into the neuron-manage --register-vote command.
Fixes # (issue)
How Has This Been Tested?
Manual testing and an automated test.
Checklist: