Skip to content

Update tabular_writer to include option to not sort rows #3578

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

avdudchenko
Copy link

Fixes # .

tabular_writer forces sorting on the user provided Rows even when user might not desire such sorting to occur, this adds an option to disable sorting

Summary/Motivation:

I want to be in control of my table order.

Changes proposed in this PR:

  • adds option to disable sorting of rows.

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@mrmundt
Copy link
Contributor

mrmundt commented Apr 28, 2025

@avdudchenko - Oop, can you please undo your last commit? You used black . instead of black -S -C ., which introduced a bunch of non-functional changes.

@avdudchenko
Copy link
Author

@avdudchenko - Oop, can you please undo your last commit? You used black . instead of black -S -C ., which introduced a bunch of non-functional changes.

Done.

@mrmundt mrmundt changed the title Update tabluate_table to include option to not sort rows Update tabular_writer to include option to not sort rows Apr 28, 2025
@blnicho blnicho requested a review from jsiirola April 29, 2025 18:46
@jsiirola
Copy link
Member

jsiirola commented May 1, 2025

Thinking about this PR, I am wondering if we should remove the sorting from tabular_writer all together? That was originally there because of Python < 3.7 when dict ordering was nondeterministic. Now that we are past that, the sort is no longer necessary for determinism. One path forward could be to:

  • remove sorting from tabular_writer
  • move the sort to the methods that generate the data dict for the tabular_writer. Right now, that is just in various implementations of display and pprint.
    • this would let us leverage the "deterministic" mode for component iteration as the default
    • we could even expose the sorting to the user through a sort= option to pprint/display (using the SortComponents standard

@jsiirola
Copy link
Member

jsiirola commented May 1, 2025

One additional thought: this would break backwards compatibility for the generated ostream, but not functional compatibility. This change could impact tests that rely on diffing those strings.

@avdudchenko
Copy link
Author

One additional thought: this would break backwards compatibility for the generated ostream, but not functional compatibility. This change could impact tests that rely on diffing those strings.

I honestly think keeping backwards compatibility is better, especially if we are concerned about tests.

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