-
Notifications
You must be signed in to change notification settings - Fork 21
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
Can --format markdown
produce legit markdown tables?
#245
Comments
https://github.com/ordo-one/TextTable/tree/master?tab=readme-ov-file#fancygrid It looks like we are currently using https://github.com/ordo-one/TextTable/tree/master?tab=readme-ov-file#simple It looks like the https://github.com/ordo-one/TextTable/tree/master?tab=readme-ov-file#pipe I believe
|
https://github.com/astanin/python-tabulate/blob/v0.9.0/tabulate/__init__.py#L472-L491 It looks like the https://github.com/ordo-one/TextTable/blob/master/Sources/TextTable/PipeFormat.swift#L33-L38 I'm guessing we can hack this code in the |
Maybe adding a new |
I don't mind changing the existing format for markdown, it was always intended to be aimed at GitHub really. |
@hassila Do you know if anyone is already working on that? I can try! Do you know anything about the upstream status of the I could try a diff on the |
That sounds good, I don't think the original TextTable is maintained - no updates for a long time, but I don't think you need to touch TextTable - it has a customisation hook:
So if you do a custom style that is Github markdown and in make a PR that utilises that new style in markdown pretty printing output mode, I think things should be good? The ascii/markdown printout paths are the same with some conditional printing for markdown really, so it should just be a couple of conditionals in there to use the new style. |
@hassila After taking a closer look… I think that the existing What is your strategy for unit testing the whole print flow? How would I test a diff (other than manually launching benchmarks from command line)? |
No nice testing strategy for print output formats I'm afraid, just manual testing really. But in this case, I think the change should be as simple as changing |
- table.print(scaledResults, style: Style.fancy)
+ table.print(scaledResults, style: format == .markdown ? Style.pipe : Style.fancy) @hassila Probably this should be good enough. Is there a specific benchmark already defined in the package that is good enough for testing? |
Yeah, that should do it (but I believe there are a few places that needs fixed in the same file for delta vs normal etc). Easiest way to test manually is just to run from command line and copy-paste into a dummy GitHub repo with a markdown file and preview it there |
@hassila Hmm… I think I could have a diff ready to migrate those I'm a big fan of unit testing… but if we feel like this code is overall stable and we don't expect many more changes this year then maybe it's for the best to keep things as they are. But if you had been planning to make things testable or wanted improved testability to move faster I might have some ideas on a refactoring. What are your thoughts about that? |
I in general do like unit testing too, but maybe put that on pause for this specific code for the moment - no changes planned really (during the remainder of this I'd expect primarily bug fixes and possibly new metrics, then likely move to Swift 6 and migrate tests to the new testing infrastructure that is in the pipeline - that might be a good time to revisit testing of the text output and would be happy to have some help with that when we get there). |
@hassila Sounds good. I think I could have the small diff with manual testing in place early next week. Thanks! |
Done in #247 , thanks! |
Hi! I'm trying to run some benchmarks with the
--format markdown
option:https://github.com/Swift-CowBox/Swift-CowBox-Sample-Data/blob/main/2024-04-25-1-struct.txt#L81-L98
This looks like it is producing the default Benchmarks "table" with markdown backticks (```). Which is good… but different than producing a first-class markdown table.
https://github.com/ordo-one/package-benchmark/blob/1.23.1/Plugins/BenchmarkTool/BenchmarkTool%2BPrettyPrinting.swift#L130-L132
Is there currently an option to format that table in markdown? Has this been discussed or tracked before? Thanks!
The text was updated successfully, but these errors were encountered: