Skip to content

[Console] Add markdown format to Table #59657

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

Merged
merged 1 commit into from
Feb 14, 2025
Merged

Conversation

amenk
Copy link
Contributor

@amenk amenk commented Jan 30, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #17466
License MIT

Aim: allow rendering markdown tables. Those don't have a Outside border frame line like --------, so we allow to remove them and define the format "markdown".

Our motivation is to pipe symfony console output in CI to GitHub/gitlab merge requests and format them properly.

@carsonbot
Copy link

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

@amenk
Copy link
Contributor Author

amenk commented Jan 30, 2025

Are the failing tests related to my commit?

@@ -46,6 +46,8 @@ class TableStyle
private string $cellRowFormat = '%s';
private string $cellRowContentFormat = ' %s ';
private string $borderFormat = '%s';
private bool $intro = true;

Choose a reason for hiding this comment

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

I don't know what is a practice in Symfony to name boolean properties, but IMO it's more clear when they have is* or has* prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. Also I was thinking to only use one field "$hasFraming" and not allow setting intro and outro separately. Also I am not sure if "framing" is a good name.
Thoughts?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm failing to get what intro/outro concepts are for. Can you please update the description of your PR to show some examples? Maybe some test cases are missing also?

edit: please add a line in the changelog of the component also

@amenk
Copy link
Contributor Author

amenk commented Feb 5, 2025

@nicolas-grekas naming is probably not so good, thanks for the suggestion.

It's simply for omitting the first and the last line (------) of the table to be able to render proper markdown (see the test case)

@amenk
Copy link
Contributor Author

amenk commented Feb 6, 2025

I changed the naming.

Remark: fabbot.io is suggesting this patch https://fabbot.io/report/symfony/symfony/59657/20ecdd1a0473ceaa5fb792b900ab41877da35a53 but I did not change those lines (and changing them would break the PHPunit tests)

@fabpot
Copy link
Member

fabpot commented Feb 14, 2025

Thank you @amenk.

@fabpot fabpot merged commit 736ef64 into symfony:7.3 Feb 14, 2025
9 of 11 checks passed
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Console] Render markdown valid using the Table class
5 participants