Skip to content

Conversation

@atravitz
Copy link
Contributor

@atravitz atravitz commented Apr 23, 2025

downstream of #1244

I'm open to making this a --pretty flag if we want to preserve backwards compatibility. This would be a good idea if users regularly use >> output.csv instead of -o output.csv. This isn't anything we suggest in the docs, so I'm not sure if it's something people actually do.

Checklist

  • Added a news entry

Developers certificate of origin

@atravitz atravitz force-pushed the add_pretty_format branch from 55cd89a to f3c92a3 Compare April 23, 2025 16:11
@atravitz atravitz requested review from IAlibay and mikemhenry and removed request for IAlibay April 23, 2025 16:20
@atravitz atravitz self-assigned this Apr 23, 2025
@atravitz atravitz requested a review from IAlibay April 23, 2025 16:20
@mikemhenry
Copy link
Contributor

I vote it is pretty by default and we can add a --tsv option for those who like to > for scripting (that way we can also do stuff like make sure nothing else ends up in stdout that would break the table)

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

News item!

Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

Not sure if you just haven't written tests yet (since it is a draft) but I think that is the only thing missing

@mikemhenry
Copy link
Contributor

Oh yes and +1 for a news item, but that can wait until the dust settles on how we want to do this and what new flags exist

@atravitz atravitz added this to the v1.4.0 milestone Apr 23, 2025
@atravitz atravitz force-pushed the add_pretty_format branch 3 times, most recently from 42b0c7a to e695053 Compare April 23, 2025 18:30
Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

looks good! One test question and needs a news item but other than that LGTMM!

@codecov
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.73%. Comparing base (6b3f501) to head (aee55ec).
Report is 258 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1246      +/-   ##
==========================================
- Coverage   92.75%   92.73%   -0.03%     
==========================================
  Files         141      141              
  Lines       10813    10845      +32     
==========================================
+ Hits        10030    10057      +27     
- Misses        783      788       +5     
Flag Coverage Δ
fast-tests 92.73% <100.00%> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mikemhenry
Copy link
Contributor

mikemhenry commented Apr 23, 2025

oh and a test for the rich printing as well

@atravitz
Copy link
Contributor Author

oh and a test for the rich printing as well

this is tricky because I'll need to mock the terminal width, since that affects text wrapping. do you know how to do this off the top of your head? otherwise i'll open an issue to follow up later, and leave it as a smoke test for now.

@atravitz atravitz force-pushed the add_pretty_format branch from f875b9f to 5004dbf Compare April 23, 2025 22:53
@atravitz atravitz marked this pull request as ready for review April 23, 2025 22:58
@atravitz atravitz requested a review from mikemhenry April 23, 2025 22:58
@jameseastwood jameseastwood assigned mikemhenry and unassigned atravitz Apr 24, 2025
@mikemhenry
Copy link
Contributor

Fine with a smoke test, this is how you can capture the output:

from io import StringIO
from rich.console import Console
console = Console(file=StringIO())
console.print("[bold red]Hello[/] World")
str_output = console.file.getvalue()

I think we have a few "levels" of testing:

  1. The strictest would be to compare the whole output to what we expect
  2. The second would be just checking if certain key numbers are in the output
  3. The lowest is just a smoke test that the makes sure the call works

I am happy with 3, but I think if 1 takes you 30 minutes it would be worth it.

@github-actions
Copy link

No API break detected ✅

Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

Oh btw since the smoke test is in there, I will give this an approval so it can get merged in now

@atravitz atravitz merged commit e42432d into main Apr 24, 2025
13 checks passed
@atravitz atravitz deleted the add_pretty_format branch April 24, 2025 17:02
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.

4 participants