Argument name should be lowercase

[dn]

Thanks for the thoughts!


On 11/11/2022 21.54, dn wrote:
> PyCharm is warning against using an identifier of all upper-case letters 
> as a function's parameter, saying "Argument name should be lowercase". 
> (weak, code smell)
> 
...
PEP-008: makes no mention, and is but a guide anyway.
(that said, if one 'breaks a rule', then a good reason should be required!)

YELLING is an email-interpretation. Does it apply to code? Particularly 
as we are talking Python, where an identifier using all upper-case 
letters is a convention (not a language-structure, as stated in 
contributions 'here') that is widely used.

Accordingly, the purpose of the UPPER_CASE is to communicate the 
read-only nature of the data-item to the programmer. Combine this with 
"we're all adults here", and if someone is foolish-enough to modify a 
'constant', then Python will comply - but (s)he can expect push-back 
during a Code Review! So, yes, «weird to see an all-cap parameter» (I 
thought so too) but isn't that effect the purpose of the convention in 
the first place?

There are several aspects of Python where the 'we're all adults' 
thinking must be applied, eg 'private attributes'. Given that 
understanding, it doesn't seem necessary to force constant-behavior on 
the parameter - although when one does, there are a couple of mechanisms 
and more than several projects working on this and other aspects of 
enforcing data-types and their characteristics!

That said, and with @Stefan's observations, there are many reasons why 
this code is piling-up trouble - and thus, should be avoided...


> def build_product_prices_dictionary( WORKBOOK_DEFINITIONS:dict )->dict:
> ...
>      price_array = xl.iget_array(
>          file_name=WORKBOOK_DEFINITIONS[ "file_name" ],
>          ...
> 
> (the function def is flagged, as above)

> As far as the function is concerned, the dict and its contents are 
> constants.
> (but the dict can't be treated as a global ENV[IRONMENT] object, because 
> it has to cross into the module's namespace)

By way of background, in recent times, have the habit/standardised code 
'template' for config/environment-setting, which instantiates a class 
from (at least) JSON/YAML files. Accordingly, the instance is named in 
lower-case, and particular groups of parameters (as here) can be passed 
as an extract-function/property (as a general style, I avoid long 
parameter/argument lists, preferring a data-collection - which landed me 
in this...).

Which re-raises the question the other way around: how does the object's 
name indicate its constant/read-only nature to the reader?
"Ouch" says idealism!

Back to this case, which part of a tutorial comparing different methods 
to access prices in a product-catalog[ue] (eg hard-coded dict, 
flat-file, workbook, database, etc). Some members of the group are 
either anti-OOP or OOP-untrained. Hence avoiding my personal 'good, old, 
stand-by'.

(plus it was Veterans'/Remembrance/Liberation Day, which we don't 
observe (much) here (ANZAC Day instead) and I was using some 
creative-coding time to [try to] take my mind off it)

The particular @Rob and I both find it peculiar (in the same sense) but 
as-above, that's (ultimately) the point of the upper-casing.

The idea of moving the workbook-definitions into that module appears to 
make sense and could easily be achieved - but in the singular context of 
this example. However, as a matter of style, all of an application's 
environment-setting would best be done in one place - with cmdLN, 
config-file(s), etc, all being combined, and with the output of a single 
'authority' as its objective.

I could try to 'get away with it', but if some Apprentice takes the idea 
and later uses it as a 'template', a general grumpiness will result... 
(no it's not a learning-objective, but sometimes people take-away 
unintended 'side effects', maybe like this).

However, in that spirit, am contemplating use of a DataClass. It will be 
as easy to read as any config file, even to non-OOP-ers; has no learning 
side-effect down-side (that I've spotted, as-yet); and can be 'seen' as 
a instance by the workbook module (instance named using lower-case). 
This, hopefully also measuring-up to @Thomas' observations of risk that 
the current code's intentions be misconstrued later.

I'll admit to (idealists look away now!) not being 'above' the use of 
globals, particularly?if only an ENVIRONMENT class - IIRC this 
irresponsible behavior being when the environment is fairly trivial (cf 
multi-layered or large numbers of 'tunable factors').

Because the ENVIRONMENT is returned to the mainline, which subsequently 
uses those parameters to call the 'action' functions/methods (and know 
which to call), it is not available within the import-ed module's 
namespace. Hence the fall-back to passing it/the Workbook sub-set of 
parameters, as an argument. (see above for normal/traditional 'solution' 
to traversing namespaces)


> Is passing the dict as an argument/parameter considered to be 
> incompatible with its designation as a constant?

There's an argument to be made, particularly by those from other 
languages, relating to pass-by-reference, pass-by-value; which suggests 
that the WORKBOOK_DEFINITIONS parameter is effectively being used on the 
LHS of an assignment...

Probably 'nuff said'!


> Perhaps the style should be more enum-like, ie the dict's name in 
> lower-case, with the key-named in upper case, eg
> 
>      workbook_definitions[ "FILE_NAME" ]

Like @MRAB, I'm thinking that getting tangled-up in the conventions of 
constant identification, cf wanting to convey the read-only nature of 
the construct is unhelpful (and an unneeded headache from over-thinking).

Changing the dict-keys to upper-case encourages the 'constant/read-only' 
interpretation, and might cause least distraction to the 
learning/discussion/comparison objectives...


> Advice, comments, critique welcome!

-- 
Regards,
=dn