Open awf opened 3 years ago
n.b. coordinate with RLO https://github.com/awf/knossos/blob/8b53d51d3a738f131ba289b9fc782370664b421a/src/rlo/sparser.py if we do make this change
Here's the best "community expectations" I could find:
https://softwareengineering.stackexchange.com/questions/171296/staticmethod-vs-module-level-function https://stackoverflow.com/questions/11788195/module-function-vs-staticmethod-vs-classmethod-vs-no-decorators-which-idiom-is
Another datapont, pylint as configured https://github.com/awf/knossos/pull/1627#issuecomment-868670330 objects to to the classproperty
approach - pylint probably needs some more guidance somehow to treat it like staticfunction
src/rlo/type.py:21:4: E0213: Method should have "self" as first argument (no-self-argument)
src/rlo/type.py:21:4: R0201: Method could be a function (no-self-use)
I quite like the namespacing of "Type.Float". Also, I like that before #882, every "Type.Float" returned the same instance, whereas IIUC, after #882, each read of Type.Float returns a new instance. (Can we combine @classproperty with @cachedproperty ??)
But, trying to construct a simple example, akin to the situation before #882:
class Foo:
def __init__(self, s):
self.s = s
def __str__(self):
return f"Foo<{self.s}>"
Foo.ex = Foo("ex")
print(Foo.ex)
running this prints "Foo
test.py:6: error: "Type[Foo]" has no attribute "ex"
test.py:7: error: "Type[Foo]" has no attribute "ex"
Found 2 errors in 1 file (checked 1 source file)
However, the following patch makes mypy happy:
@@ -1,3 +1,4 @@
class Foo:
+ ex: "Foo"
def __init__(self, s):
self.s = s
is that another option, or do other people dislike the namespacing of the old approach (i.e. #882 is not just about mypy) ?
Yes, the change will have caused there to be more allocation for e.g. Type.Float
.
That looks like another reasonable alternative. As a variation of that, that's more typed (when in Type do as the Typers do...):
class Type
# ...
Float: "Type"
# ...
# After
Type.Float = Type("Float")
Is much closer to the original code and achieves much of the same - mypy / pylance pass, tests pass. Keeps performance of the "primitives" e.g. Type.Float
. Has the downside of separating the attribute from it's intended definition and risk of someone doing one without the other, and also away from the other constructors e.g. Type.Tensor
I'm very happy to do this latest version, sorts out types, passes pylint but also happy with the larger change of module functions if folks prefer that.
I admit to a slight cosmetic preference for Type.Float
but we shouldn't let that override more important concerns - what do others think @awf @ryotatomioka ?
I think I prefer either type.Type. -> type. or the last one proposed by Alan. Classproperty seems too complicated for the goal (make mypy happy)
We've merged @acl33 suggestion as a simpler option than classproperty
, can potentially still move to type.* approach suggested by @sarahnlewis at a later date.
Closed by #909 ?
Closed by #909 ?
Depends if we want to consider the more disruptive type.* change that avoids splitting initializing the attributes and defining them.
With #909 it all type checks, passes tests, passes pylint (well, the equivalent change in RLO) so I'm happy to consider it closed.
Thanks @cgravill ! So on the question of type.Type. -> type., just a quick observation:
from ksc.type import Integer, Float
would mean int
and float
were python types (used in type annotations, for example) and Integer
and Float
were ksc types (python objects). I guess that's OK. In contrast:
from ksc import type
probably not so good, as that shadows the builtin function type
.
Post #882, all type constructors are static methods on class
Type
in moduletype
. Clients thenWe could lift them to module level to make the API
What are the pros and cons of such a change?