Skip to content

Preserve element order when initializing Python dictionary #57

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

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

philipturner
Copy link
Contributor

Pandas DataFrames were rendering incorrectly in IPython. This bug was reported in philipturner/swift-colab#22, and seems to have been around in the swift-jupyter era. The culprit is how a dictionary literal currently converts into a Python object.

public init(dictionaryLiteral elements: (PythonObject, PythonObject)...) {
self.init(Dictionary(elements, uniquingKeysWith: { lhs, _ in lhs }))
}

The new implementation preserves element order in the final Python object, unlike Dictionary.pythonObject. When keys are duplicated, throw the same runtime error as Swift.Dictionary.init(dictionaryLiteral:). This differs from Python's key uniquing semantics, which silently override an existing key with the next one it encounters.

The previous implementation used Dictionary.init(uniquingKeysWith:) and retained only the first duplicate key's value. Changing the rule introduces API breakage, but is the most semantically correct. We want to create a Swift-first API, and duplicating keys in a literal is bad/unsafe practice.

Additional changes

  • Fixed malformatted comment above prefix func -.
  • Adjusted Members initializer of PythonClass to follow the uniqueness semantics above. The alternative initializer uses Dictionary, which up until now had different behavior upon key duplication.

@philipturner
Copy link
Contributor Author

I also have a working private notebook where I modified the PythonKit embedded into Swift-Colab, and Pandas DataFrames rendered correctly.

@pvieito pvieito merged commit 060e1c8 into pvieito:master Sep 1, 2022
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.

2 participants