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

Adds the simpleEnum class back. #246

Merged
merged 4 commits into from
Mar 17, 2023
Merged

Adds the simpleEnum class back. #246

merged 4 commits into from
Mar 17, 2023

Conversation

cheqianh
Copy link
Contributor

@cheqianh cheqianh commented Mar 13, 2023

Description of changes:

  • Adds its meta class back without using six.
  • Adds unit tests back
  • Marks it as deprecation. Adds deprecation warnings for the class.

Issue #, if available:
#241

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* Adds its meta class back without using six.
* Adds unit tests back
* Adds a deprecation warning for the class.
Comment on lines 307 to 310
warn(f'{self.__class__.__name__} was deprecated in favor of the deprecation of `amazon.ion.Enum`. However, for '
f'compatibility reasons, `amazon.ion.Enum` has been placed here. We recommend considering using `IntEnum` '
f'as an alternative.',
DeprecationWarning, stacklevel=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend some stronger language both here and in the other warning.

Suggested change
warn(f'{self.__class__.__name__} was deprecated in favor of the deprecation of `amazon.ion.Enum`. However, for '
f'compatibility reasons, `amazon.ion.Enum` has been placed here. We recommend considering using `IntEnum` '
f'as an alternative.',
DeprecationWarning, stacklevel=2)
warn(f'{self.__class__.__name__} is an internal-only class in ion-python; do not use it for any reason. '
f'This class is deprecated and may be removed without further warning in any future release. '
f'Use `IntEnum` instead.',
DeprecationWarning, stacklevel=2)

Copy link
Contributor Author

@cheqianh cheqianh Mar 14, 2023

Choose a reason for hiding this comment

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

Agree, changed - 3a4708e



class Enum(int, metaclass=_EnumMetaClass):
"""Simple integer based enumeration type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Simple integer based enumeration type.
"""This is a deprecated, internal-only class in ion-pythondo NOT use it for any reason.
Simple integer based enumeration type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.



class _EnumMetaClass(type):
"""Metaclass for simple enumerations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it obvious in the doc string too.

Suggested change
"""Metaclass for simple enumerations.
"""This is a deprecated, internal-only class in ion-pythondo NOT use it for any reason.
Metaclass for simple enumerations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@cheqianh
Copy link
Contributor Author

cheqianh commented Mar 14, 2023

Found the root why CI/CD fails, see #249. We will skip these 10 failed unit tests for now.

This PR passed all tests except these 10 known tests, which is good.

@popematt
Copy link
Contributor

Looks like the build is failing in the Python 3.10 workflow because of an error while trying to load the c extension. Do you know why that might be?

@cheqianh
Copy link
Contributor Author

cheqianh commented Mar 14, 2023

The CI/CD will fail for all python 3.x versions due to the recent ion-c release. Looks like it's because of this commit. Further investigation is required to identify the root cause. Reference issue - #249 for details.

So we can either

  1. Trigger the workflow by using ion-c 1.1.0, which I tested locally and didn't see any error
  2. Wait until the issue solved, but it takes some time to investigate the root

ion-c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's revert the submodule update and address that in a separate PR, since it introduced errors that haven't yet been diagnosed.

@cheqianh
Copy link
Contributor Author

The CI/CD passed due to the mitigation we had #250, which the commit will be reverted once the root cause is fixed.

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.

3 participants