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

feat: turn enumwrappers into real python enumerators #570

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Conversation

qdelamea-aneo
Copy link
Contributor

@qdelamea-aneo qdelamea-aneo commented Nov 18, 2024

  • feat: turn enumwrappers into real python enumerators
  • fix: correctly convert TaskOptions option property into Python dictionnary

Motivation

So far, the API wrappers for enumerators are not “real” Python enumerators. This makes it difficult to obtain the name associated with the value of enumerator elements.

Description

This change fixes this problem by making each enumerator wrapper a “real” Python enumerator.

It also fixes a minor bug in the conversion of the TaskOptions option field to a native Python dictionary.

Testing

A few manual tests in addition to unit tests.

Impact

This changes should have no impact.

Additional Information

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • I have thoroughly tested my modifications and added tests when necessary.
  • Tests pass locally and in the CI.
  • I have assessed the performance impact of my modifications.

Copy link

github-actions bot commented Nov 18, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1478 1248 84% 0% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 12a884e by action🐍

@dbrasseur-aneo
Copy link
Contributor

dbrasseur-aneo commented Nov 18, 2024

I'm fine with the changes, with 2 caveats:

  • Why remove the "name from value" ? Displaying that the status is "2" is not all that helpful (and that's most likely what you'll have when constructing the task from the grpc message most likely). You may need to properly convert the int to the enum when constructing from a message, with perhaps an overload of __str__ on the enums to display it properly
  • Not sure the ruff change belongs here

@qdelamea-aneo
Copy link
Contributor Author

qdelamea-aneo commented Nov 21, 2024

I'm fine with the changes, with 2 caveats:

  • Why remove the "name from value" ? Displaying that the status is "2" is not all that helpful (and that's most likely what you'll have when constructing the task from the grpc message most likely). You may need to properly convert the int to the enum when constructing from a message, with perhaps an overload of __str__ on the enums to display it properly
  • Not sure the ruff change belongs here

You're right!

  • To avoid any breakage or other problems, I finally kept the name_from_value methods.
  • For Ruff configuration, I created a separate pr (see #572).

@qdelamea-aneo qdelamea-aneo merged commit 334b694 into main Nov 21, 2024
59 checks passed
@qdelamea-aneo qdelamea-aneo deleted the qd/enum branch November 21, 2024 11:07
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