-
Notifications
You must be signed in to change notification settings - Fork 213
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
Implement transaction list
command in CLI
#467
Comments
|
Regarding:
I think I can see a problem with using these particular parameter names. ProblemCurrently, specifying There are two cases:
Therefore, I'd like to propose the following options: Option 1: Rename the command line argumentsWe could rename the arguments to Examples:
Option 2: Make the order explicit in the CLIRequire that This option has the drawback that it would restrict the CLI to only ever returning results in ascending order. However, we can overcome this restriction by adding a third argument:
QuestionOut of these options, which would people prefer? |
Yes, definitely |
Good point. No particular preference between |
Use `--start` and `--end` instead of `--after` and `--before`. See: #467 (comment)
The range header shouldn't be overloaded for specifying sort direction (see my comment in #466). The names "before" and "after" would be fine if it weren't for this unexpected behaviour. |
@rvl wrote:
From the above link:
I think this is a good point. But fixing this would require us to change the @KtorZ what are your thoughts on this? |
Use `--start` and `--end` instead of `--after` and `--before`. See: #467 (comment)
We could indeed make the ordering explicit by extending the format with an A few examples:
|
What do you think? |
Hi @KtorZ @rvl and I just had a discussion about this. We feel it might be better to encode these arguments as query parameters. For example: /wallets/<wallet-id>/transactions
/wallets/<wallet-id>/transactions?start=<time>
/wallets/<wallet-id>/transactions?end=<time>
/wallets/<wallet-id>/transactions?start=<time>&end=<time>
/wallets/<wallet-id>/transactions?start=<time>&end=<time>&order=ascending
/wallets/<wallet-id>/transactions?start=<time>&end=<time>&order=descending&limit=10 Advantages:
@KtorZ Would you be okay with this scheme? I can make the relevant changes, if you're happy with us to go ahead. |
Well, to be honest, I think query parameters shouldn't be used to convey meta-information like pagination in a REST API; This is abusing the HTTP specification which already provides mechanism for such things (like request headers, in particular For the record, I am quite fond of what Heroku's engineering team did with their API regarding pagination and versionning:
Having said that, if both of you guys thinks that it'd be better to go for query parameters instead, I won't fight too much over it and will refrain my "principled approach" 😬 ... A couple of things to keep in mind though:
Let's do this @rvl @jonathanknowles |
Hi @KtorZ
Thanks for sharing your perspective. I actually wasn't aware of this point of view.
Also, thanks for sharing this. I'll take a look. As for:
I was under the impression that the To check this assumption, I coded up the
So it seems that URI encoding is handled already, unless I have missed something obvious?
I'll make sure to not include anything like this. |
Hello all! I just wanted to say that I'm a big fan of the If you implement an iOS |
Context
Follow-up for #466 and #465. We do want to provide this capability in the CLI too, in a very similar fashion to what we currently do with other commands.
Decision
This ticket can in practice starts after #465 but can only be completed once #466 is done. The CLI will support filtering through two extra options
--start
&--end
both optional and accepting a date in ISO-8601 UTC format but no wildcard (the absence of option has the semantic of a wildcard).The transaction history should be displayed in JSON format, as we do for addresses.
Acceptance Criteria
transaction list
command must be implemented in the CLItransaction list
command must provide an option to specify an upper-bound for filtering.transaction list
command must provide an option to specify a lower-bound for filtering.transaction list
command must provide an option to specify a sort order, either ascending or descending.2.
should be named--start
.3.
should be named--end
.4.
should be named--order
.*
.Development Plan
transaction list
command to the CLI.transaction list --help
displays the expected help text.transaction list
returns[]
for a brand new wallet, regardless of what values are provided for--start
or--end
(as expected).--start
nor--end
are specified.--start
or--end
are specified (or both).PR
master
master
QA
The text was updated successfully, but these errors were encountered: