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

Undeprecate literal_(de)stringizer #6943

Merged

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Sep 22, 2023

I propose to rescind the deprecation of gml.literal_stringizer and gml.literal_destringizer.

These functions have been deprecated since 2.5 but were never removed because it is not possible to do so without breaking tests. I spent a frustrating couple hours diving into this and gained a few insights. Currently, literal_stringizer is necessary to handle two specific cases that are tested:

  1. Graphs with nodes that are tuples, and
  2. Attributes that include nested sequences

The first one is relatively easy to "fix" without literal stringizer. For completeness sake, it would involved adding the following snippet:

elif isinstance(value, tuple) and key == "label":
    yield indent + key + f" \"({','.join(repr(v) for v in value)})\""

before L761.

The 2nd one is really tough to crack. The default stringizer function has an in_list function to decide how to handle elements inside lists, but this only works for "1D" lists - nested sequences break this pattern and will end up raising an exception. literal_stringizer gets around this by only unpacking the outermost list, then directly repr-ing everything inside it. I'm not even sure that the GML standard supports nested lists as attributes! However, this currently "works" according to test_data_types, even if the result isn't actually valid, standards-compliant gml.

IMO, the simplest thing to do would be to revert the deprecation. On the off-chance that there are users depending on this behavior, literal_stringizer is currently the only simple way to achieve it. It looks like the original motivation for deprecating the literal_*stringizer functions was Python 2->3 cleanup, but these corner cases are still relevant and unrelated to Python 2. Furthermore, I think reverting the deprecation will be relatively painless, as the deprecation warnings aren't currently visible due to the stack level.

@rossbar rossbar added Discussion Issues for discussion and decision making type: API labels Sep 22, 2023
@rossbar rossbar force-pushed the undeprecated-literal-stringizer branch from 8082754 to 1bc7cdd Compare September 25, 2023 05:28
Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Thanks Ross!
I appreciate the effort spent diving into these functions. :)

Copy link
Member

@jarrodmillman jarrodmillman left a comment

Choose a reason for hiding this comment

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

Thanks!! I spent too much time to trying to sort this out as well.

@jarrodmillman jarrodmillman merged commit 47b36de into networkx:main Sep 25, 2023
38 checks passed
@jarrodmillman jarrodmillman added this to the 3.2 milestone Sep 25, 2023
Alex-Markham pushed a commit to Alex-Markham/networkx that referenced this pull request Oct 13, 2023
Undeprecate literal_(de)stringizer.
dschult pushed a commit to BrunoBaldissera/networkx that referenced this pull request Oct 23, 2023
Undeprecate literal_(de)stringizer.
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
Undeprecate literal_(de)stringizer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues for discussion and decision making type: API
Development

Successfully merging this pull request may close these issues.

None yet

3 participants