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

Add proper support for constant aliases #25

Open
JennDahm opened this issue Nov 11, 2019 · 0 comments
Open

Add proper support for constant aliases #25

JennDahm opened this issue Nov 11, 2019 · 0 comments

Comments

@JennDahm
Copy link

(Apologies in advance if I'm not using the right terminology, and for the wall of text.)

I'd like to be able to add aliases for some of my constants. As an example, I'm writing a TFTP library, and the TFTP spec uses some terminology that might be unfamiliar to users, so I'd like to be able to alias the older terms to newer, more familiar terms, while still giving access to the old terms as well. There seem to be a couple ways to do this, but neither has the full behavior I want.

What I want to see is for each constant, regardless of whether it's an alias or not, to report itself in __repr__() (i.e. self.name) as the name it was declared with, but also for constants with the same value to report themselves as equal. I don't especially care about sorting order in this case, but it would make sense to me to order them according to their values rather than their declared order.

The first way to declare aliases is as follows:

class TransferMode(constantly.Values):
    """Representation of the different transfer modes. Also includes some convenient aliases.

    The various transfer modes are defined in RFC 1350.
    """
    NETASCII = constantly.ValueConstant("netascii")
    OCTET = constantly.ValueConstant("octet")
    MAIL = constantly.ValueConstant("mail")
    ASCII = constantly.ValueConstant("netascii")
    TEXT = constantly.ValueConstant("netascii")
    BINARY = constantly.ValueConstant("octet")
>>> TransferMode.NETASCII
<TransferMode=NETASCII>
>>> TransferMode.OCTET
<TransferMode=OCTET>
>>> TransferMode.MAIL
<TransferMode=MAIL>
>>> TransferMode.ASCII
<TransferMode=ASCII>
>>> TransferMode.TEXT
<TransferMode=TEXT>
>>> TransferMode.BINARY
<TransferMode=BINARY>
>>> TransferMode.NETASCII == TransferMode.ASCII
False

This gives me the first part (each constant reports itself as the name it was declared with), but it doesn't give me the second part (that is, TransferMode.NETASCII == TransferMode.ASCII is False when I'd like it to be True).

The second way to do aliases is as follows:

class TransferMode(constantly.Values):
    """Representation of the different transfer modes. Also includes some convenient aliases.

    The various transfer modes are defined in RFC 1350.
    """
    NETASCII = constantly.ValueConstant("netascii")
    OCTET = constantly.ValueConstant("octet")
    MAIL = constantly.ValueConstant("mail")
    ASCII = NETASCII
    TEXT = NETASCII
    BINARY = OCTET
>>> TransferMode.NETASCII
<TransferMode=TEXT>
>>> TransferMode.OCTET
<TransferMode=OCTET>
>>> TransferMode.MAIL
<TransferMode=MAIL>
>>> TransferMode.ASCII
<TransferMode=TEXT>
>>> TransferMode.TEXT
<TransferMode=TEXT>
>>> TransferMode.BINARY
<TransferMode=OCTET>
>>> TransferMode.NETASCII == TransferMode.ASCII
True

When I do this, I get TransferMode.NETASCII == TransferMode.ASCII like I want, but now NETASCII and ASCII report themselves as TEXT, when I would expect them to report themselves as NETASCII and ASCII, respectively. I don't see the same thing happen to OCTET after declaring BINARY = OCTET, but BINARY does report itself as OCTET. The sorting order seems to be kept the same and I can still look up all of the constants by name. Placing the declaration of BINARY a line before the declaration of TEXT doesn't change the sorting behavior, so it's not quite like the constant is being treated as though it were redefined where TEXT was defined.

TransferMode.iterconstants() reports all six declared constants, but with the wrong names for half of them.

>>> TransferMode.lookupByName("NETASCII")
<TransferMode=TEXT>
>>> TransferMode.lookupByName("OCTET")    
<TransferMode=OCTET>
>>> TransferMode.lookupByName("MAIL")  
<TransferMode=MAIL>
>>> TransferMode.lookupByName("ASCII") 
<TransferMode=TEXT>
>>> TransferMode.lookupByName("TEXT")  
<TransferMode=TEXT>
>>> TransferMode.lookupByName("BINARY") 
<TransferMode=OCTET>
>>> TransferMode.NETASCII < TransferMode.OCTET
True
>>> TransferMode.NETASCII < TransferMode.MAIL 
True
>>> TransferMode.OCTET < TransferMode.MAIL
True
>>> TransferMode.NETASCII == TransferMode.TEXT
True
>>> TransferMode.OCTET == TransferMode.BINARY
True
>>> for c in TransferMode.iterconstants():
...  print(c)
... 
<TransferMode=TEXT>
<TransferMode=TEXT>
<TransferMode=TEXT>
<TransferMode=OCTET>
<TransferMode=OCTET>
<TransferMode=MAIL>

Removing the declaration of TEXT reverts NETASCII to reporting itself as NETASCII, but now ASCII reports itself as NETASCII, too. It seems to indicate that the ValueConstant instance reports itself as whichever of its aliases is last in the English dictionary.


Flags similarly doesn't support aliasing, but it has an additional problem. Consider the following (highly simplified) example:

class Test(constantly.Flags):
  A = constantly.FlagConstant(1)
  B = constantly.FlagConstant(2)
  C = constantly.FlagConstant(1)
>>> Test.A | Test.C
<Test={A,C}>
>>> Test.A ^ Test.C
<Test={A,C}>
>>> ~Test.B
<Test={A,C}>

This is definitely not what should happen. Even ignoring the weird naming, Test.A ^ Test.C should be <Test={}>, not <Test={A,C}>! Those flags are no longer set! Using the second declaration style has correct bitwise results here, but it still has the same problems as ValueConstants.

It's also bad when one of the constants is a combination of other flags.

class Test(constantly.Flags):
  A = constantly.FlagConstant(1)
  B = constantly.FlagConstant(2)
  C = constantly.FlagConstant(3)
>>> Test.A | Test.C
<Test={A,C}>
>>> Test.A ^ Test.C
<Test={A,C}>
>>> ~Test.B
<Test=A>

~Test.B behaves properly now, and Test.A | Test.C and Test.A ^ Test.C behave like the last example. We can't even get there with the second declaration style, though, as declaring C = A | B complains AttributeError: 'FlagConstant' object has no attribute 'names' because A and B haven't been properly set up yet.


Regarding an actual solution to this, I think the easiest way to implement this for ValueConstants would be to redefine the comparison operators to compare self.value rather than self._index. That would sufficiently allow aliases using the first declaration method, though it would change the comparison ordering (IMO to something that makes better sense, but it is a noticeable interface change). It also still leaves the aliases listed in iterconstants(), which isn't ideal.

Because FlagConstants also have a value attribute, that implementation would begin to apply to them, too, in the most basic cases, but it still has the problem with bitwise operations. It also doesn't work very well if you're creating an alias representing multiple flags being set. Making aliases work properly for FlagConstant may require some overhaul of its internals.

NamedConstants don't have an associated value, so you'd have to add one to properly mimic the equality functionality. That could probably be as simple as a new alias_of() function (or class?) that copies the original constant's _index to the new alias. Equality checks would then compare _index first. This would also group all aliases of the same constant together when iterating over the class, and also give a good way to filter out aliases, which can apply to the other Constant types as well.

How feasible is this to add? The current behavior leaves a bit to be desired.

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