Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11654 )
Change subject: paging: Add VTY options to calculate T3113 timeout dynamically ...................................................................... Patch Set 5: Code-Review-1 (4 comments) I don't like this patch but if you fix the string issues I'm not blocking anymore. https://gerrit.osmocom.org/#/c/11654/3/src/osmo-bsc/bsc_vty.c File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/#/c/11654/3/src/osmo-bsc/bsc_vty.c@3916 PS3, Line 3916: "T-number, optionally preceded by 't' or 'T'.") > Following your logic, it's clear that in "foo bar?", ? applies only to last > parameter, and not to wh […] This is about the user typing on the vty and reading the online help. I think above example did show that user experience. I still think this isn't nice: > no timer-dynamic? Calculate T3113 dynamically based on channel config and load because in all cases I know, the help string is useful for the entire command issued so far, not assuming that the user is manually negating what is being said. https://gerrit.osmocom.org/#/c/11654/5/src/osmo-bsc/bsc_vty.c File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/#/c/11654/5/src/osmo-bsc/bsc_vty.c@3940 PS5, Line 3940: "Calculate T3113 dynamically based on channel config and load\n" as I said, I still think this is misleading and not nice. If you really must do this, then at least define a common FOO_STR instead of duping the char constant. But I'd still urge you to rather fix the vty online doc for the 'no' case. https://gerrit.osmocom.org/#/c/11654/5/src/osmo-bsc/bsc_vty.c@3941 PS5, Line 3941: "T-number, optionally preceded by 't' or 'T'.") here is another FOO_STR candidate. https://gerrit.osmocom.org/#/c/11654/5/src/osmo-bsc/bsc_vty.c@3955 PS5, Line 3955: vty_out(vty, "T%d has no dynamic%s", d->T, VTY_NEWLINE); so you also disagree with the other thing I said... -- To view, visit https://gerrit.osmocom.org/11654 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4fb2969b690151415038631fb6ad059aa6835c7f Gerrit-Change-Number: 11654 Gerrit-PatchSet: 5 Gerrit-Owner: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Stefan Sperling <[email protected]> Gerrit-Comment-Date: Tue, 04 Dec 2018 17:11:33 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
