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 4: Code-Review+1 (2 comments) hmm, if stsp also agrees, maybe I should insist on my hunch that a generic T1234 dynamic setting will not make sense anyway. But I don't want to block this more than necessary. Arguing for my earlier points... just a +1 from me for now because I think they are valid points. 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'.") > I didn't completely get what you mean here regarding docs, but anyway that > sounds confusing to me wh […] Point 1 is, we often have #defines to re-use the exact same string again, because only one doc string survives in the online docs. DEFUN("foo one", "Foo to set one\n" "One arg") DEFUN("foo two", "Foo to set two\n" "Two") > list foo one foo two > foo? Foo to set one > foo ? one One arg two Two So in that case, you want a FOO_STR that is always the same. But when there is a 'no' before that, that's not an issue. This example shows that, and also illustrates the other point: #define FOO_STR "Set a foo value\n" DEFUN("foo one", FOO_STR "One arg") DEFUN("no foo", NO_STR FOO_STR) > list foo one no foo > foo? Set a foo value > no? Negate a command or set its defaults > no ? foo Set a foo value > no foo? Set a foo value "no foo" now says that it sets a value. For me it makes more sense to describe what "no foo" would actually do: DEFUN("no foo", NO_STR "Disable foo") > no? Negate a command or set its defaults > no ? foo Disable foo > no foo? Disable foo So > no timer-dynamic? Calculate T3113 dynamically based on channel config and load that's "WTF" to me. https://gerrit.osmocom.org/#/c/11654/3/src/osmo-bsc/bsc_vty.c@3927 PS3, Line 3927: return CMD_WARNING; > Sure, but anyway nowadays doesn't make sense, to let's better tell the user > probably he's doing some […] (still think it makes sense to say "has no dynamic" when the user tries to set a dynamic value; if you really need to say something here, maybe rather "T%d already is non-dynamic"?) -- 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: 4 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, 27 Nov 2018 01:33:55 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
