Fixed order parameter in the listforwards command

[vincenzopalazzo]

From a report in IRC channel, I noted that the order of the parameters are wrong in the rpc command.

I assumed that the code take the wrong order because for us (programmers) write documentation is costly more than code :-)

In addition, I think is more natural to have as the first parameter the status, in my opinion, it is more frequently have a filter by the status instant of the filter by channel id.

In conclusion, I can revert the change and modify the docs, or if this change is good, I'm happy to know if the place where I wrote the test is a correct place (test_peers.py or test_rpc.py file is missed).

[rustyrussell]

I think we need to fix the documentation, not the code unfortunately, as this was already released in 0.10.0 so people may rely on it.

[vincenzopalazzo]

@rustyrussell I will open a new PR with the doc fixing :) thanks

[vincenzopalazzo]

Applied the suggestion of @rustyrussell and test it locally. However, I did not find a method to test the deprecated API function in the python test.

[cdecker]

Wasn't the resolution for this to update the docs instead of reordering the arguments in code? As is this is pretty much a breaking change as @rustyrussell pointed out.

[vincenzopalazzo]

Hi @cdecker, Yes you have right, but we discussed with @rustyrussell on IRC https://gnusha.org/c-lightning/2021-07-20.log and this change make the API of this command more user friendly. All the code inside this PR is made by rusty suggestion.

[cdecker]

Ok, sounds good. Will merge it then.

[cdecker]

ACK e560b69

[cdecker]

Rebased on top of master to ensure we havent accidentally broken things.

[cdecker]

Had to move the autodata declaration to avoid having a linenumber conflict...

[cdecker]

This is the IRC log extract for posterity:

11:01 < nathanael> hmm `lightning-cli listforwards settled` should work imo - synopsis according to the manpage: listforwards [status] [in_channel] [out_channel] - i get an error saying it needs a short_channel_id
12:39 < vincenzopalazzo> <nathanael "hmm `lightning-cli listforwards "> I think that the order of the parameter is wrong, or maybe the description? However, this PR should be contains a solution https://github.com/ElementsProject/lightning/pull/4668
13:09 -!- rich- is now known as yakshaver_
13:11 -!- yakshaver_ is now known as yakshaver
14:32 < rusty> nathanael: nice catch, yes, doc is wrong.  Unf it was in previous release so changing order to match docs is a breaking change (vs. changing docs).  Commented on PR vincenzopalazzo
14:33 < vincenzopalazzo> rusty: Ops, I will close the PR and open a new one. i missed to check the changelog, sorry
14:34 < rusty> vincenzopalazzo: I used git blame to get commit 8a8f81175d, then git describe --contains 8a8f81175d returned v0.10.0rc1~166
14:35 < rusty> I mean, we *could* change order, but it's messy :(  I agree that status first is probably nicest.  lightning-cli listforwards status=settled is less nice.
14:38 < vincenzopalazzo> rusty: Yes agree to have the status before, but if something is already in production is better to leave it as is. angry people are bad for API change are bad :)
14:39 < rusty> We have a 2-release / 6 month window for deprecating old APIs.  And we can detect this (status and short-channel-id are easy to distinguish).  But it's a bit messy.  nathanael, vincenzopalazzo: thoughts?
14:40 < rusty> (API changes are painful in the short term, but bad APIs are painful forever!)
14:41 < vincenzopalazzo> rusty: the first think that this quotes need to be in a book (API changes are painful in the short term, but bad APIs are painful forever!)
14:41 < vincenzopalazzo> for the api, I think we can open an issue and change it later?
14:42 < vincenzopalazzo> in another major version? like 0.11.0
14:42 < rusty> Well, if we change it right now, it's only been that way for one release.
14:42 < rusty> (And that means *today*, since we are about to release rc1!)
14:42 < rusty> niftynei: ?
14:43 < vincenzopalazzo> this is also true
14:44 < rusty> Let me quickly hack something together for review, while we think if this is worthwhile...
14:45 < vincenzopalazzo> So, I will reopen the PR
15:09 < rusty> vincenzopalazzo: untested, but something like this? https://0bin.net/paste/aEXqITFF#t1vJmCnoeWKneWjpU6GbLswFBWqFooRZ5ffYftJC47F
15:09 < rusty> (gtg, my kids are waking up!)
15:17 < vincenzopalazzo> <rusty "vincenzopalazzo: untested, but s"> ack, to deprecate the API and move on with a API change. i you want I can work on your draft and integrate it in the PR?
16:26 < rusty> vincenzopalazzo: please do!
16:27 < rusty> (Test might be nice, ofc! :)

[rustyrussell]

Ack 791a4ea