Skip to content

Proof of concept for replacing int in constants.py with IntEnums#56

Draft
sliva0 wants to merge 2 commits intomasterfrom
IntEnum_tests
Draft

Proof of concept for replacing int in constants.py with IntEnums#56
sliva0 wants to merge 2 commits intomasterfrom
IntEnum_tests

Conversation

@sliva0
Copy link
Copy Markdown
Collaborator

@sliva0 sliva0 commented Mar 3, 2026

This branch is proof of concept for replacing E_* int constants in constants.py with enum.IntEnum-based classes.

Pros:

  • AVP text description (and Message dumps) contain names of the options from the specifications alongside non-descriptive number like 2001. Example:
MIP-Feature-Vector <Code: 0x151, Flags: 0x40 (-M-), Length: 12, Val: 32 (MN_FA_KEY_REQUEST)>
  • Possibility to validate if Enumerated AVP values are in specified range.
  • Possibility to make constants namespace much smaller, my LSP is struggling when there is a star import from it right now.

Cons:

  • Backwards compatibility is complicated. IntEnum is based on int and inherits almost all behaviours from it, but not all, so if user is doing something stupid, like type(...) == int - code might break.

There are other possible implementations of this proposal - this PoC shows the easiest way to introduce it - by defining enums in constants.py, making old names reference new names, and then updating Message.from_bytes and AvpEnumerated, so they always return IntEnum values (it also defines things where they shouldn't just for simplicity).

  • Encoding doesn't need any updates either way, it already can work with both ints and IntEnums.
  • Decoding can be easily updated to return IntEnums only when some flag is passed to Message.from_bytes or globally set on library import. Same or separate flag can make it to throw exception when trying to decode message with unknown enumerated values.
  • new IntEnum classes can be defined in separate files from current constants.py, completely separate from already defined int constants, although this adds additional maintenance burden to keep them in sync.

In my opinion first and third pros worth it to make decoding optionally convert ints to enums, but I'm open to other suggestions, there may be some nuances that I don't see.

@mensonen
Copy link
Copy Markdown
Owner

I've finally had the chance to go through this and I do like it a lot. This approach to the enum AVPs should have been there from the start and I think we should merge this.

Regarding backwards compatibility - it looks compatible enough to me, especially as you have kept the old enum constants still in constants.py and aliased them to the new enum values. Anyone doing type checking on enum values is not using the stack in its intended way anyway, I don't think we have to try to cover that option.

The only thing this pull request doesn't really fulfill, is what you set out as one of your initial goals: "Possibility to make constants namespace much smaller". This does, in fact, make the namespace even bigger and now brings also my IDE to a halt when I try to edit the file. Things run better, if I disable all inspections and linting for that file, but maybe you could look into moving the Enums to their own separate file already now?

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

Successfully merging this pull request may close these issues.

2 participants