Skip to content
This repository has been archived by the owner on Oct 4, 2024. It is now read-only.

Mixed-case case names not accepted by .match(), but README says they're fine #41

Open
kodo-pp opened this issue Dec 3, 2020 · 0 comments

Comments

@kodo-pp
Copy link

kodo-pp commented Dec 3, 2020

The README says:

It's conventional to declare your fields with ALL_UPPERCASE names, but the only true restriction is that they cannot be lowercase.

If I understand this correctly, it means that the only kind of names that are invalid are names that are fully in lowercase. That is, FOO_BAR, FooBar, and foo_Bar are valid names, but foobar and foo_bar aren't.

However, match method does not seem to work nicely with names that are not fully in uppercase. The following code is supposed to work:

from adt import adt, Case

@adt
class Foo:
    Bar: Case
    Baz: Case

value = Foo.Bar()
print(value.match(
    bar=lambda: 'bar',
    baz=lambda: 'baz',
))

But it fails with the error: ValueError: Unrecognized case BAR in pattern match against <<class '__main__.Foo'>.Bar: None> (expected one of dict_keys(['Bar', 'Baz'])).

The code of match seems to convert the names of variants to upper case, which may not be correct when original names contain lowercase letters (provided that such names are allowed).

I see two possible solutions here.

  1. Forbidding variant names not in the format ALL_UPPERCASE, stating this in the README and perhaps enforcing this rule in the decorator in order to prevent accidental mistakes. This may be backwards-incompatible, but is probably the easiest solution. As a compromise, it is possible to deprecate names containing lowercase characters and possibly to warn about them in the decorator.
  2. Modifying the code to accept non-fully uppercase names. This is tricky because we either have to continue accepting case-insensitive variant names in match and somehow convert them to their original case, or accept only names in their original case (that is, one would only be able to write something like value.match(Bar=..., Baz=...), not value.match(bar=..., baz=...)). The former may be tricky to implement, and the latter would probably totally break backward compatibility.

Out of these two, I personally prefer the first solution, but it's just my opinion on this problem.

P.S. I used the term variant as a synonym of case here to avoid confusion between lower/upper case and ADT cases.

kodo-pp added a commit to AndroidM0nkey/bot_arena that referenced this issue Dec 3, 2020
Currently, tests are failing, because the `adt` library is supposedly
designed to work only with ALL_UPPERCASE case names, and I used
PascalCase names. This wasn't initially clear from the documentation,
and this was reported upstream as an issue:
jspahrsummers/adt#41

The obvious solution is to change case names to ALL_UPPERCASE, which
is probably what I will do soon.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant