rbw / aiosnow

Asynchronous ServiceNow Library
MIT License
73 stars 12 forks source link

Own dunders #65

Closed trosos closed 4 years ago

trosos commented 4 years ago

I suspect that snow does not comply with PEP 8 in that it uses its own __double_leading_and_trailing_underscore__ names (specifically, it expects attribute __location__ in snow.resource.Schema subclasses).

I would like not to be forced to disobey the official style guide and use dunders for things external to Python core.

rbw commented 4 years ago

There's nothing about this in PEP8, and flake8 doesn't complain about it.

However, it is mentioned in the official documentation here: https://docs.python.org/3/reference/lexical_analysis.html#reserved-classes-of-identifiers

What do you think about _location instead?

rbw commented 4 years ago

Another alternative could be to move it to the Resource constructor and use it like:

async with app.resource(Incident, location="...") as r:
    ...

This adds some (probably not needed?) flexibility.

trosos commented 4 years ago

Citing PEP 8:

__double_leading_and_trailing_underscore__: "magic" objects or attributes that live in user-controlled namespaces. E.g. __init__, __import__ or __file__. Never invent such names; only use them as documented.

I would not be happy with _location either, as this is reserved for "private" members (again citing from PEP 8):

_single_leading_underscore: weak "internal use" indicator. E.g. from M import * does not import objects whose names start with an underscore.

I propose to establish a similar interface to what Django uses for model metadata:

class Incident(Schema):
    class Meta:
        location = "/api/now/table/incident"

    sys_id = fields.Text(is_primary=True)
    description = fields.Text()
    impact = fields.Numeric()

(The implementation might want to employ metaclasses, but it it is probably not needed.)

rbw commented 4 years ago

I propose to establish a similar interface to what Django uses for model metadata:

That's much better, thanks. Not sure why I didn't think of that considering marshmallow, which is used by snow, uses a Meta inner class as schema options object.

_single_leading_underscore: weak "internal use" indicator. E.g. from M import * does not import objects whose names start with an underscore.

That refers to the module object in the context of the python import system, while this is about a protected class member. Anyway, this was mainly to separate options from schema fields used in serialization, etc, and I didn't think of using an inner class.

rbw commented 4 years ago

Should be fixed with #69, please review and/or test it out if you can.

rbw commented 4 years ago

This was also a thing in snow.request, fixed with https://github.com/rbw/snow/commit/ee496878e9bf3b5424bb4f6a0bf3cc2bf8288cc8