Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isinstance(attr, Attr) #29

Open
boisgera opened this issue Sep 2, 2021 · 4 comments
Open

isinstance(attr, Attr) #29

boisgera opened this issue Sep 2, 2021 · 4 comments

Comments

@boisgera
Copy link
Owner

boisgera commented Sep 2, 2021

will fail even when attr is a correctly formed tuple. Attr is useful as a piece of documentation since in the REPL one can do for example

>>> Image
Image(Attr, [Inline], Target)
>>> Attr
Attr = (Text, [Text], [(Text, Text)])

but the attributes produces by a pandoc read is a mere tuple, not an Attr instance. Actually Attr cannot be instantiated at all. So while the documentation aspect is handy (to avoid the inlining of Attr definition everywhere it is used), the existence of the class may also be misleading since it cannot be used in pattern matching.

@boisgera
Copy link
Owner Author

boisgera commented Sep 2, 2021

Consider using some custom __instancecheck__ class method (at the metaclass level) for Typedef instances to have the proper behavior [*] ? We could check structural conformity of the "instance", say check that we have a tuple of length 2 with two Text components and thus consider that it's a valid Target. But pattern-matching the whole document for such pattern could still lead (theoretically) to false positives (tuples that just happen to have a structure consistent with Target). A real example would be : we have a key-value component of an Attr ; this is not a target, but would pattern-match. So such pattern-matching would be unsafe ...

So the "proper" way to deal with this would be not to match for Typedefs, but match the parents in the type hierarchy instead. That's easy for Target (only two parents : Image and Link). This is less acceptable for Attr : they are used by a great number of types and the Attr argument is not even always in first position (for Header, this is the second argument ...). So a general "find all Attrs and do something with them" would require some significant amount of boilerplate.

[*} See https://docs.python.org/3/reference/datamodel.html#customizing-instance-and-subclass-checks

@boisgera
Copy link
Owner Author

boisgera commented Sep 2, 2021

So such pattern-matching would be unsafe ...

We could consider making an instance check on a Typedef throw an exception, instead of allowing a construct which won't do what the user is expecting, for non-obvious reasons.

@boisgera
Copy link
Owner Author

boisgera commented Sep 7, 2021

Well actually there are two sides to this:

  • pattern matching on typedefs is a lousy, unsafe practice, so that pleads for an error on isinstance,

  • but an isinstance that implements some structural typechecking would be handy for typechecking.

Since we are all consenting adults ... structural typecheking would be the way to go. Plus a decent amount of documentation to teach that it should be avoided in pattern matching. There is already a footnote on this subject in the cookbook.

@boisgera
Copy link
Owner Author

boisgera commented Sep 8, 2021

The underyling logic should already be present in the typecheck branch actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant