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

Fix "first" sort for inline tables #77

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

vfazio
Copy link
Contributor

@vfazio vfazio commented Nov 19, 2024

Previously, sorting inline tables would throw a ValueError if the key in
the table had trailing spaces.

For example:

  [a]
  b = {c=1, d = 3}

Sorting a.b with "d" first would fail.

Removing the space would "pass":

  [a]
  b = {c=1, d=3}

However, when the table was written back out, rerunning toml-sort would
fail because the keys are written with surrounding spaces:

  [a]
  b = {c = 1, d = 3}

When commit 9900c71 was introduced, the value used for the index
lookup in the list of "first" keys changed to be Key.as_string().

The intent was probably to pass type checks as the list of keys are
strings and mypy complains when doing an index lookup that doesn't match
the typing of the list elements (str vs Key).

However, as_string() uses the "original" value of the key which
includes spaces and is subtlely different than the check performed
on the previous line that checked if the value was in the list.

Both the in keyword and the list.index method use the equality
operation to evaluate if a value is in a list.

The Key object defines the __eq__ method that compares the internal
key value (which is stripped) to the other value. This is why the in
lookup succeeds but the index lookup fails.

Now, the lookup logic is simplified.

Instead of potentially searching the list of "first" keys twice, once
via in and once via list.index, iterate the list via enumerate to
keep track of the index and perform an explicit equality check against
Key.key which appeases mypy.

Add a test case to cover overriding the "first" key for inline tables.

Signed-off-by: Vincent Fazio <[email protected]>
Previously, sorting inline tables would throw a ValueError if the key in
the table had trailing spaces.

For example:

  [a]
  b = {c=1, d = 3}

Sorting a.b with "d" first would fail.

Removing the space would "pass":

  [a]
  b = {c=1, d=3}

However, when the table was written back out, rerunning toml-sort would
fail because the keys are written with surrounding spaces:

  [a]
  b = {c = 1, d = 3}

When commit 9900c71 was introduced, the value used for the index
lookup in the list of "first" keys changed to be `Key.as_string()`.

The intent was probably to pass type checks as the list of keys are
strings and mypy complains when doing an index lookup that doesn't match
the typing of the list elements (str vs Key).

However, `as_string()` uses the "original" value of the key which
includes spaces and is subtlely different than the check performed
on the previous line that checked if the value was in the list.

Both the `in` keyword and the `list.index` method use the equality
operation to evaluate if a value is in a list.

The `Key` object defines the `__eq__` method that compares the internal
key value (which is stripped) to the other value. This is why the `in`
lookup succeeds but the `index` lookup fails.

Now, the lookup logic is simplified.

Instead of potentially searching the list of "first" keys twice, once
via `in` and once via `list.index`, iterate the list via `enumerate` to
keep track of the index and perform an explicit equality check against
`Key.key` which appeases mypy.

Fixes: 9900c71 ("Modernize tests, fix test errors")
Closes: pappasam#76
Signed-off-by: Vincent Fazio <[email protected]>
@pappasam
Copy link
Owner

🚀 thanks, this lgtm!

@pappasam pappasam merged commit 1d986cc into pappasam:main Nov 19, 2024
6 checks passed
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