Re: [nft v2 PATCH 3/3] py: add input_{set,get}_flags() API to helpers

[Phil Sutter <phil@nwl.cc>]

Hi Thomas,

On Fri, Jul 14, 2023 at 10:48:53AM +0200, Thomas Haller wrote:
> Note that the corresponding API for output flags does not expose the
> plain numeric flags. Instead, it exposes the underlying, flag-based C
> API more directly.
> 
> Reasons:
> 
> - a flags property has the benefits that adding new flags is very light
>   weight. Otherwise, every addition of a flag requires new API. That new
>   API increases the documentation and what the user needs to understand.
>   With a flag API, we just need new documentation what the new flag is.
>   It's already clear how to use it.
> 
> - opinionated, also the usage of "many getter/setter API" is not have
>   better usability. Its convenient when we can do similar things (setting
>   a boolean flag) depending on an argument of a function, instead of
>   having different functions.
> 
>   Compare
> 
>      ctx.set_reversedns_output(True)
>      ctx.set_handle_output(True)
> 
>   with
> 
>      ctx.ouput_set_flags(NFT_CTX_OUTPUT_REVERSEDNS | NFT_CTX_OUTPUT_HANDLE)
> 
>   Note that the vast majority of users of this API will just create one
>   nft_ctx instance and set the flags once. Each user application
>   probably has only one place where they call the setter once. So
>   while I think flags have better usability, it doesn't matter much
>   either way.
> 
> - if individual properties are preferable over flags, then the C API
>   should also do that. In other words, the Python API should be similar
>   to the underlying C API.
> 
> - I don't understand how to do this best. Is Nftables.output_flags
>   public API? It appears to be, as it has no underscore. Why does this
>   additional mapping from function (get_reversedns_output()) to name
>   ("reversedns") to number (1<<0) exist?

I don't recall why I chose to add setters/getters for individual output
flags instead of expecting users to do bit-fiddling. Maybe the latter is
not as common among Python users. :)

On the other hand, things are a bit inconsistent already, see
set_debug() method. 

Maybe we could turn __{get,set}_output_flag() public and make them
accept an array of strings or numbers just like set_debug()? If you
then adjust your input flag API accordingly, things become consistent
(enough?), without breaking existing users.

FWIW, I find

| ctx.set_output_flags(["reversedns", "stateless"])

nicer than

| ctx.set_output_flags(REVERSEDNS | STATELESS)

at least with a Python hat on. WDYT?

Cheers, Phil