Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Wed, Mar 25, 2020 at 10:08:25AM +0100, Christoph Zwerschke wrote:
>
> Am 25.03.2020 um 02:20 schrieb Justin Pryzby:
> > The old stuff can be removed eventually. I like this:
> >
> > if self.escaping_funcs and pg_version >= (9, 0):
> > define_macros.append(('ESCAPING_FUNCS', None))
> > That allows a packager to disable a feature
> > if they want the abilityto run> against old libpq.
>
> The problem is that these switches are by default set to true. The idea was
> that you set them to false on the command line if you don't want these
> features to be compiled. However, I think that never worked since you simply
> can't set these flags to false on the command line, you only can set them to
> true by specifying the names of the options (i.e. pass --escaping-funcs as
> option).
>
> So we need to change that and instead use the negatives of these flags
> everywhere as options i.e. "no_escaping_funcs" in this case.
I think I would leave the "positive" variable names, and *add* negative forms
of the commandline options. --no-foo and/or --foo=no
Also, I believe it used to be a best-practice for distribution packagers to
specify --with-foo --with-bar --with-baz even if those were the defaults (or if
they defaulted to true if libfoo/bar/baz were installed). That avoids silently
building packages with reduced functionality if libfoo isn't found at compile
time. So logic like this might be nice:
if self.enable_memsize and pg_version < (12, 0):
error('memsize not supported until pg12')
postgres vacuum options have a ternary system: on/off/default. If you need
more than a line of logic, maybe you'd write:
if self.enable_memsize is True and pg_version < (12, 0):
error('memsize not supported until pg12')
else if self.enable_memsize == -1: # default/unset
self.enable_memsize = bool(pg_version >= (12, 0):)
...
if self.enable_memsize is True:
define_macros.append(('WITH_MEMSIZE', None))
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 25.03.2020 um 02:20 schrieb Justin Pryzby:
> The old stuff can be removed eventually. I like this:
>
> if self.escaping_funcs and pg_version >= (9, 0):
> define_macros.append(('ESCAPING_FUNCS', None))
> That allows a packager to disable a feature
> if they want the abilityto run> against old libpq.
The problem is that these switches are by default set to true. The idea
was that you set them to false on the command line if you don't want
these features to be compiled. However, I think that never worked since
you simply can't set these flags to false on the command line, you only
can set them to true by specifying the names of the options (i.e. pass
--escaping-funcs as option).
So we need to change that and instead use the negatives of these flags
everywhere as options i.e. "no_escaping_funcs" in this case. I usually
avoid negative forms since "Can I not have my coffee with no sugar"
sounds silly when I can say "Can I have coffee with sugar". But in the
case where "sugar" is always the default, it's ok, since you only need
to say "no sugar please" and never need to ask for sugar explicitly. (I
hope this was not confusing :)
-- Christoph
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 25.03.2020 um 10:34 schrieb Justin Pryzby:
I think I would leave the "positive" variable names, and *add* negative forms
of the commandline options. --no-foo and/or --foo=no
Yes, that would be nice. Unfortunately, the options are not interpreted
by us, but by distutils. The way the options are currently defined in
PyGreSQL they act as booleans that do not take arguments, i.e. you can
set --foo, but not --foo=false. Unfortunately how the options are
interpreted is not well documented (at least I find nothing), so I had
to look into the Python source and found distutils uses getopt to parse
the raw options. Therefore it is possible to require arguments by
writing a "=" after the name of the argument in user_options, e.g.
('escaping-funcs=', None, "enable string escaping functions"),
When defined that way, we will be able to pass "--escaping-funcs=no" or
"--escaping-funcs false" as an option. When 'escaping-funcs' is added to
boolean_options, then the argument needs to be something like 0/false/no
or 1/true/yes and will be converted to a Python bool.
Unfortunately, getopt does not allow default arguments. So it looks like
we have to choose between:
1) Either two options --escaping-funcs and --no-escaping-funcs, both
without arguments or
2) one option --escaping funcs with a mandatory boolean argument
In both cases, when we set self.escaping_funcs = None in
initialize_options, we can implement the ternary on/off/default.
Not sure whether 1) or 2) is better, what do you think?
-- Christoph
___
PyGreSQL mailing list
PyGreSQL@Vex.Net
https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
On Wed, Mar 25, 2020 at 12:25:12PM +0100, Christoph Zwerschke wrote: > Am 25.03.2020 um 10:34 schrieb Justin Pryzby: > > I think I would leave the "positive" variable names, and *add* negative > > forms > > of the commandline options. --no-foo and/or --foo=no > > 1) Either two options --escaping-funcs and --no-escaping-funcs, both without > arguments or > 2) one option --escaping funcs with a mandatory boolean argument > > In both cases, when we set self.escaping_funcs = None in initialize_options, > we can implement the ternary on/off/default. > > Not sure whether 1) or 2) is better, what do you think? I think (1) is more common, I think that's what autoconf and gnu tools use. -- Justin ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
Re: [PyGreSQL] [PATCH] PQresultMemorySize
Am 25.03.2020 um 12:28 schrieb Justin Pryzby: I think (1) is more common, I think that's what autoconf and gnu tools use. In that case, maybe we should use enable/disable or with/without as prefixes, like autotools? But I think it's not very common as setup option. Also, by using the old option name and just adding the "no-" variant, we would stay backward compatible. -- Christoph ___ PyGreSQL mailing list PyGreSQL@Vex.Net https://mail.vex.net/mailman/listinfo/pygresql
