Skip to content

Conversation

mmacata
Copy link
Contributor

@mmacata mmacata commented May 21, 2025

This PR suggests to add a new print format in CSV table style. Module and imports are listed, separated by ;

Example:

$ findimports --table

apple;os
apple;os.path
apple;sys
apple;sys
box.cat;gc
box.cat;box.yarn
box.cat;decoy
findimports;argparse
findimports;ast
findimports;doctest
...

@mgedmin
Copy link
Owner

mgedmin commented May 22, 2025

Thank you for your contribution!

I don't mind adding a CSV output format, but why use ; as separator? CSV stands for Comma-Separated Values, after all.

Also, --table to me implies an ASCII-art formatted table for human consumption, so please use --csv or something.

(And this needs unit tests.)

@mmacata
Copy link
Contributor Author

mmacata commented May 22, 2025

Thank you for your contribution!

I don't mind adding a CSV output format, but why use ; as separator? CSV stands for Comma-Separated Values, after all.

Also, --table to me implies an ASCII-art formatted table for human consumption, so please use --csv or something.

(And this needs unit tests.)

Thank you for your quick response!

I also had --csv in mind, but the -c was already taken 😬 That's why I came up with --table.

In the new suggestion I use --tabular and keep the -t. What do you think? I also thought about --table-csv.

And I replaced the ; with ,. I know the abbreviation 🙃 but as most applications or libraries have options to specify the delimiter, I ususally prefer ;. But in this context here, a , is very unlikely to appear in a field, so it is fine for me.

Also tests are now added.

@mgedmin
Copy link
Owner

mgedmin commented May 23, 2025

Wait, -c is already used? I missed that when I looked at the options. -C could still be used.

I was thinking maybe --output=csv or --output-format=csv? -o csv for short, if -o is not taken yet. (This could later be generalized to add -o json and other formats.)

@mmacata
Copy link
Contributor Author

mmacata commented May 27, 2025

Allright, I would go with -C then.

@mmacata
Copy link
Contributor Author

mmacata commented Jun 2, 2025

@mgedmin do you agree now?

@mgedmin mgedmin merged commit 5da690f into mgedmin:master Jun 2, 2025
12 checks passed
@mgedmin
Copy link
Owner

mgedmin commented Jun 2, 2025

Thank you, merged.

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