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

pprint and repr now sort map keys #1698

Closed
wants to merge 1 commit into from

Conversation

krader1961
Copy link
Contributor

This change is more invasive than I expected because the existing Elvish value comparison code was in pkg/eval. This moves the value comparison logic into pkg/eval/vals so it can be used by packges other than eval. For example, the vals package repr implementation as well as other Elvish modules which may want to compare Elvish values.

I also decided to introduce a strutil.Dedent function to make it easier to write multiline string literals in unit tests to improve readability of the expected value.

This also sorts the keys of pseudo (struct) maps for consistency with mutable maps. This arguably makes the output of pretty-printing pseudo maps less friendly since a struct map definition typically orders its members in a deliberate fashion (unlike a regular map). However, it is more important that the output of printing the output of regular and pseudo maps (via repr or pprint) be consistent with regard to the order of their keys.

Related: #1495

@krader1961
Copy link
Contributor Author

This also modifies the compare and order commands by introducing a &types option so they can be used to compare (order) otherwise uncomparable types. This is for symmetry with the behavior of repr and pprint when outputting maps. I have had conflicting opinions about this. My original change had the compare and order commands always impose a total ordering on uncomparable types. I have decided it makes more sense for users of compare and order to have to explicitly ask for ordering based on the type of otherwise uncomparable values while having repr and pprint implicitly order based on value and type.

@krader1961
Copy link
Contributor Author

Regarding the introduction of a Dedent function see my comment regarding new literal string syntax. The Dedent functionality is useful outside of its use in the unit tests of this pull-request.

This change is more invasive than I expected because the existing Elvish
value comparison code was in `pkg/eval`.  This moves the value comparison
logic into `pkg/eval/vals` so it can be used by packges other than
`eval`. For example, the vals package `repr` implementation as well as
other Elvish modules which may want to compare Elvish values.

I also decided to introduce a `strutil.Dedent` function to make it
easier to write multiline string literals in unit tests to improve
readability of the expected value.

This also sorts the keys of pseudo (struct) maps for consistency with
mutable maps. This arguably makes the output of pretty-printing pseudo
maps less friendly since a struct map definition typically orders its
members in a deliberate fashion (unlike a regular map). However, it is
more important that the output of printing the output of regular and
pseudo maps (via `repr` or `pprint`) be consistent with regard to the
order of their keys.

Related: elves#1495
@xiaq
Copy link
Member

xiaq commented May 8, 2023

The CmpTypes function does not handle all the builtin types, which includes nil, exceptions, functions and more "esoteric" ones like styled strings.

Printing maps with ordered keys does not require a total ordering of all values. You can group values by types, and only sort the groups that are actually sortable.

This also sorts the keys of pseudo (struct) maps for consistency with mutable maps. This arguably makes the output of pretty-printing pseudo maps less friendly since a struct map definition typically orders its members in a deliberate fashion (unlike a regular map). However, it is more important that the output of printing the output of regular and pseudo maps (via repr or pprint) be consistent with regard to the order of their keys.

Pseudo maps and maps are already different. I would rather pseudo maps keep their original key order.

Regarding the introduction of a Dedent function see my comment regarding new literal string syntax. The Dedent functionality is useful outside of its use in the unit tests of this pull-request.

A testutil.Dedent function already exists.

@xiaq
Copy link
Member

xiaq commented May 8, 2023

If you'd like to continue contributing this change, please break the change that moves cmp to pkg/eval/vals into a separate PR. It's a bit hard to review the PR as it is today.

You can also leave this feature to me as I have a pretty good idea of how I'd like it to work at this point.

@krader1961
Copy link
Contributor Author

The CmpTypes function does not handle all the builtin types, which includes nil, exceptions, functions and more "esoteric" ones like styled strings.

Neither does the existing cmp implementation. The obvious solution is to use reflection when one of the types is unknown. If both types are the same just treat the values as equal. This has the value of not having to explicitly enumerate all types since there isn't a natural ordering for types such as exceptions and functions. Yet it allows ordering sequences that contain such types without having to deal with an exception from the compare or order command (assuming the types are homogenous). A similar solution is available to the CmpTypes implementation; only in that case compare the type names to derive an ordering between the types.

Closing this since I'll push an update to PR #1700 that implements the idea in the previous paragraph for the vals.Cmp function.

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.

None yet

2 participants