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.

Sign up for GitHub

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

Jump to bottom

enum_to_str: convert enum keys, not just enum values #549

Merged
merged 12 commits into from
Feb 22, 2021
Merged

enum_to_str: convert enum keys, not just enum values #549

merged 12 commits into from
Feb 22, 2021

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Feb 17, 2021 •
edited
Loading

Closes #531
Closes #550

@Jasha10 Jasha10 changed the title initial patch of enum_to_str enum_to_str: convert enum keys, not just enum values Feb 17, 2021
@Jasha10 Jasha10 marked this pull request as ready for review February 17, 2021 22:58
@Jasha10 Jasha10 requested a review from omry February 17, 2021 22:58
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Feb 17, 2021

I created some tests for the enum_to_str argument to to_container.
Previously there were no tests for this.

omry
omry reviewed Feb 18, 2021
View reviewed changes
tests/test_to_container.py Outdated Show resolved Hide resolved
tests/test_to_container.py Show resolved Hide resolved
@Jasha10 Jasha10 requested a review from omry February 18, 2021 23:33
@Jasha10 Jasha10 self-assigned this Feb 19, 2021
@Jasha10
refactor test_to_container.py
e2d3b42 
- use `from pytest import ...` rather than `import pytest`
- revert TestEnumToStr and make the tests more compact
@Jasha10
use key directly in test_enum_to_str_for_values
910e4d9
omry
omry reviewed Feb 20, 2021
View reviewed changes
Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good. a few nits.

tests/test_to_container.py Outdated Show resolved Hide resolved
tests/test_to_container.py Outdated Show resolved Hide resolved
tests/test_to_container.py Outdated Show resolved Hide resolved
@omry
Copy link
Owner

omry commented Feb 20, 2021

Oh, I thought about #550 a bit:
Every bugfix is a behavior change, I don't think this one justifies an API change entry. (I doubt anyone have actually ran into it).

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Feb 20, 2021

I don't think this one justifies an API change entry.

Ok, I'll scrap the #550 news fragment.

Jasha10
Jasha10 commented Feb 20, 2021
View reviewed changes
tests/test_to_container.py Outdated Show resolved Hide resolved
@Jasha10 Jasha10 requested a review from omry February 21, 2021 01:22
omry
omry approved these changes Feb 22, 2021
View reviewed changes
Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks.

@omry omry merged commit e2c60a2 into omry:master Feb 22, 2021
@Jasha10 Jasha10 deleted the to_yaml-convert-Enum-key branch February 22, 2021 22:23
odelalleau pushed a commit to odelalleau/omegaconf that referenced this pull request Mar 26, 2021
@omry @odelalleau
SQUASHED STUFF
5807cf5 
comments

fixed docs

fixed notebook

enum_to_str: convert enum keys, not just enum values (omry#549)

README: Fix typo in "What's new" url

rebase against master

separate tests for struct mode and non-struct mode

news fragment for omry#554

one key per supported key type

Co-authored-by: Omry Yadan <[email protected]>

test None value for struct flag

special case parametrizing for struct_mode

change struct_mode test order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omry omry omry approved these changes

Assignees

@Jasha10 Jasha10

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Jasha10 @omry