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

Ingesting tabulate as an internal module #1811

Merged
merged 76 commits into from
Aug 15, 2024
Merged

Conversation

john-science
Copy link
Member

@john-science john-science commented Aug 8, 2024

What is the change?

I removed the tabulate dependency, and just ingested the parts of that tool we use to armi.utils.tabulate.

Why is the change being made?

To close #1749

The problem is that tabulate appears to be a nigh abandoned project, and it is keeping us from being able to support modern PIP versions.

(Arrielle also points out that we could just replace tabulate with something else, like prettytable. I am not super convinced my approach here is better than that one. I went this route first because it was easy for me to do, and would be easier to make the update for our downstream repos.)

Code Coverage

The code coverage on this file is pretty high:

image


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

armi/utils/tabulate.py Outdated Show resolved Hide resolved
@jakehader
Copy link
Member

jakehader commented Aug 13, 2024

I think it's fine to pull in 🐫 case throughout and then move code over to armi's tabulate as it makes sense. Should be pretty easy to see where to replace these things. Could also replace things that are 🐍 case with a simple name (tabular_data -> data or something). Should we pull in additional documentation of this new feature into the ARMI docs? For example pull in examples from https://github.com/astanin/python-tabulate?tab=readme-ov-file#library-usage?

@john-science
Copy link
Member Author

I think it's fine to pull in 🐫 case throughout and then move code over to armi's tabulate as it makes sense. Should be pretty easy to see where to replace these things. Could also replace things that are 🐍 case with a simple name (tabular_data -> data or something). Should we pull in additional documentation of this new feature into the ARMI docs? For example pull in examples from https://github.com/astanin/python-tabulate?tab=readme-ov-file#library-usage?

Most of that is already included in the docstring for the tabulate.tabulate() function:

armi/armi/utils/tabulate.py

Lines 1027 to 1043 in cac3324

r"""Format a fixed width table for pretty printing.
>>> print(tabulate([[1, 2.34], [-56, "8.999"], ["2", "10001"]]))
--- ---------
1 2.34
-56 8.999
2 10001
--- ---------
The first required argument (`data`) can be a list-of-lists (or another iterable of
iterables), a list of named tuples, a dictionary of iterables, an iterable of dictionaries, an
iterable of dataclasses (Python 3.7+), a two-dimensional NumPy array, or NumPy record array.
Table headers
-------------
To print nice column headers, supply the second argument (`headers`):

I considered putting what you linked as a module-level docstring, and reducing this ridiculously long function docstring (>200 lines). I could do that, and find any missing pieces from the docs you linked as well.

Copy link
Member

@opotowsky opotowsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are waiting to merge, but in the meantime: approved! tabulate is officially pac-manned!

@john-science
Copy link
Member Author

@jakehader I grabbed that README and put it in the module-level docstring in this file. Thanks!

@john-science john-science merged commit 9dc6760 into main Aug 15, 2024
21 checks passed
@john-science john-science deleted the substitute_tabulate branch August 15, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace tabulate with prettytable
3 participants