[sketch] Prevent typos in option tables #797

wingo · 2016-03-02T14:13:13Z

Oftentimes we pass around tables of options. Some of those options are required, some are optional. Thing is, it becomes hard to add required options over time, as old code might not detect that it needs to change; it's also hard to let old code know when options go away; and most importantly it's easy to make typos. It's also a bit of irritating effort to provide for optional values which default to false; the idiom conf.foo = conf.foo or bar is just too easy to type.

In the lwaftr and in pflua we use helpers to parse options that solve all these problems. Maybe they're good for snabb itself? Feedback welcome @lukego @eugeneia :)

I note that @kbara once thought this sort of thing was too much machinery, then it caught a nice bug and she became a believer :)

mention-bot · 2016-03-02T14:13:14Z

By analyzing the blame information on this pull request, we identified @lukego, @altexy and @javierguerragiraldez to be potential reviewers

kbara · 2016-03-02T14:18:31Z

It is too much machinery, it's just the least bad option in these contexts. ;-)

wingo · 2016-03-02T14:22:55Z

Hehe :) I admit, I really like common lisp's keyword arguments :)

eugeneia · 2016-03-03T18:29:44Z

I would prefer this to be in core.lib (and built into core.config.parse_app_args). Anything that speaks against that?

Cc @lukego

lukego · 2016-03-09T04:51:15Z

Great! I would really love to have this. I definitely have that uneasy feeling when using table parameters that are not validated for typos etc. Commonly do make mistakes on parameters to engine.run() for example.

@eugeneia Good question. To me core.lib is a bit of a disorganized mess and I shudder a bit when I read the documentation and see how much stuff we have just thrown in there and thus made part of our API. So I agree that putting this into core.lib would be consistent with normal practice but I am not sure how much I want to promote that. Up to you guys.

eugeneia · 2016-03-09T17:09:25Z

@lukego Completely agree that core.lib needs a cleanup. As parsing table configurations is a core competency of Snabb (every serious app needs to do it), I would persist on having this in core.lib and it being built into config.parse_app_arg. The current pattern to parse app configurations is absolutely horrible and this is a chance to end this swiftly. Cleaning up core.lib is a separate task.

eugeneia · 2016-07-25T14:31:41Z

I would like to have this, but this PR seems forgotten. I would offer to take over integrating this, given there is demand.

kbara · 2016-08-01T17:31:23Z

I agree; pinging @wingo .

…config # Conflicts: # src/apps/rate_limiter/rate_limiter.lua

Merge upstream v2017.04 into lwaftr

wingo added 2 commits March 2, 2016 14:53

Add lib.parameters

8d58c03

Convert rate limiter to use parameters.parse

823fcb9

Add missing include

6d9c784

eugeneia self-assigned this Mar 3, 2016

eugeneia added a commit to eugeneia/snabb that referenced this pull request Sep 7, 2016

Merge PR snabbco#797 (Prevent typos in option tables) into parse-app-…

d5f21cb

…config # Conflicts: # src/apps/rate_limiter/rate_limiter.lua

eugeneia mentioned this pull request Sep 8, 2016

config.app: parse app args #1019

Merged

eugeneia merged commit 6d9c784 into snabbco:master Oct 7, 2016

lukego pushed a commit to lukego/snabb that referenced this pull request May 22, 2017

Merge pull request snabbco#797 from Igalia/incorporate-2017-04

470f43e

Merge upstream v2017.04 into lwaftr

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sketch] Prevent typos in option tables #797

[sketch] Prevent typos in option tables #797

[sketch] Prevent typos in option tables #797

[sketch] Prevent typos in option tables #797

Conversation