Skip to content

Conversation

@kroenlein
Copy link
Collaborator

This PR removes methods that were deprecated, scheduled for removal w/ the 2.0 release.

  • Drop name argument from LinkByUID.from_entity
  • Drop type-casting methods get_value and get_enum from BaseEnumeration
  • Drop register_classes meta-method from GemdJson
  • Drop native_uid argument from substitute_links from util

@@ -1,5 +1,5 @@
"""Test serialization and deserialization of gemd objects."""
import json
import json as json_builtin
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? I don't see anything else called json, and even if there was, I'd suggest aliasing the other thing rather than the built-in.

Copy link
Collaborator Author

@kroenlein kroenlein Feb 1, 2024

Choose a reason for hiding this comment

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

Originally done b/c I'm having some weird namespace collisions in my IDE, but this didn't fix it. However, there's enough weird use-built-in-for-some-stuff-but-custom-elsewhere in this library that until that gets cleaned up that I think it's important to be clear which is being used. Maybe I'll get it cleaned up next release. Or when we change techs.

anoto-moniz
anoto-moniz previously approved these changes Feb 1, 2024
Copy link
Contributor

@anoto-moniz anoto-moniz left a comment

Choose a reason for hiding this comment

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

LGTM. Just that one comment, but it's on a test file and not a big deal.

anoto-moniz
anoto-moniz previously approved these changes Feb 2, 2024
Copy link
Contributor

@anoto-moniz anoto-moniz left a comment

Choose a reason for hiding this comment

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

As noted in Slack, I'm very opposed to the renaming of the builtin json import on the basis of clarity, and even more since it's to cater to a specific IDE.

But as I also noted, I don't care enough to argue about it, so I won't block on it.

anoto-moniz
anoto-moniz previously approved these changes Feb 5, 2024
@kroenlein kroenlein merged commit 6df6538 into main Feb 5, 2024
@kroenlein kroenlein deleted the maintain/2-0-remove-deprecations branch February 5, 2024 21:33
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