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

Remove convert method to OrderedDict #139

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Nov 24, 2024

The generic one in Julia stdlib does the job, except for the deprecation warning shown when converting from an unordered dict (such as Base.Dict) to an OrderedDict{K,V} type.

This avoids potential invalidations in packages. There also is no explanation as to why it makes sense to forbids such conversions.

The depwarn was introduced in bea4031 but I don't know why. I certainly don't want to hastily merge this without understanding this better. I am also happy to close this PR and instead open one which adds a comment with an explanation of why this conversion is deprecated, as soon as somebody explains it to me :-). Or just open a PR with that explanation and we merge that and close this PR.

CC @timholy @oxinabox who perhaps might have an idea?

Also I figure one should perhaps use nanosoldier to test a PR like this given how critical this package is in the ecosystem? but I have no idea how to do that (and maybe also not the access rights? I know too little about it)

@timholy
Copy link
Member

timholy commented Nov 25, 2024

I'm guessing it's because the user is making that conversion because they care about the order in the resulting object, but if you're converting from an unordered container the resulting order is unpredictable? But honestly I don't know.

@fingolfin fingolfin added this to the v2.0.0 milestone Nov 25, 2024
The generic one in Julia stdlib does the job, except for the deprecation
warning shown when converting from an unordered dict (such as `Base.Dict`)
to an `OrderedDict{K,V}` type.

This avoids potential invalidations in packages. There also is no explanation
as to why it makes sense to forbids such conversions.
@oxinabox
Copy link
Member

oxinabox commented Nov 26, 2024

I am fine with this.

It's not always sensible to convert an Unordered Dict to an OrderedDict, since the order of an unordered dict is arbitary. and caprecious even, since different dicts with same content can have different order, and even the order of a dict can change if you pass it over the network (JuliaLang/julia#34265).
but occationally you don't care about the order of the dictionary so long as it is consistent (like in the case you are passing it over a network), or you might want future additions to be kept in order but don't care for stuff you already have gotten unordered.

And given there is already a fallback that does the same thing (minus the warning messages) we do not have worry about breaking this.
This is fine to delete.

@fingolfin fingolfin removed this from the v2.0.0 milestone Nov 26, 2024
@fingolfin fingolfin marked this pull request as ready for review November 26, 2024 13:45
@fingolfin fingolfin mentioned this pull request Nov 26, 2024
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