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

message: Fix IntEnum tag usage #56

Merged
merged 5 commits into from Sep 8, 2023
Merged

message: Fix IntEnum tag usage #56

merged 5 commits into from Sep 8, 2023

Conversation

ghost
Copy link

@ghost ghost commented Sep 7, 2023

IntEnum were being converted to strings in fix_tag which caused issues when sourcing FIX tags from an IntEnum.

IntEnum were being converted to strings in `fix_tag` which caused issues when sourcing FIX tags from an IntEnum.
@da4089
Copy link
Owner

da4089 commented Sep 8, 2023

Thanks for the PR!

This is a good one. I tested out the scenario, and I couldn't reproduce the issue. So I added a test case and pushed it (see commit 4aa0e72) and the CI picked up that it works in 3.11 but not earlier releases.

I'm looking at this in some more detail.

@ghost
Copy link
Author

ghost commented Sep 8, 2023

Hi,

It seems like enum behavior has indeed changed in Python 3.11+: https://mail.python.org/archives/list/python-dev@python.org/message/RN3WCRZSTQR55DOHJTZ2KIO6CZPJPCU7/

I just fixed a namespace error in my pull request.

@da4089
Copy link
Owner

da4089 commented Sep 8, 2023

Rather than the explicit check for IntEnum type, I think it might be better to check for the ability to convert to an integer, like:

if hasattr(value, '__int__'):
    return str(int(value)).encode('ASCII')

The performance is more-or-less the same, and it will hopefully support anything else that comes along that can support the int() conversion.

What do you think? If that's ok for you, could you make that change, and I'll merge it.

@ghost
Copy link
Author

ghost commented Sep 8, 2023

That would be indeed much better! I just committed the updated version to the branch, you'll likely want to squash my commits when merging.

@da4089 da4089 merged commit 0ea045b into da4089:master Sep 8, 2023
@da4089
Copy link
Owner

da4089 commented Sep 8, 2023

Thanks again for this -- much appreciated!
I've pushed v1.0.16 to PyPI containing this fix.

@ghost
Copy link
Author

ghost commented Sep 8, 2023

Thank you!

@ghost ghost deleted the intenum-tag branch September 13, 2023 13:31
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.

1 participant