Skip to content

Conversation

@ElectreAAS
Copy link
Collaborator

Extracted from #12064, as it has little to do with the core of it.
Pp.text can be broken on any space, while Pp.paragraph won't.
Other than that, we have these User_message.Style.ts, let's use them

Completely subjective PR

@ElectreAAS ElectreAAS requested a review from Alizter September 3, 2025 16:10
@Alizter
Copy link
Collaborator

Alizter commented Sep 3, 2025

Regarding the placement of the failure count. Imagine you are a user and you've just seen several errors requiring you to scroll up in your terminal to see them all. If we print the count at the end the user can know before scrolling how many errors to expect, whereas if we print the count at the top, the user will have to scroll all the way too the top, which might be tricky to find, in order to see how many failures there are.

I'm not opposed to the other changes, but we should think a bit more carefully about the placement to make sure we aren't making dune harder to use than it already is.

@shonfeder
Copy link
Member

Could this include some tests showing how the output will be rendered after these changes to ensure it is working as expected?

Copy link
Member

@shonfeder shonfeder 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 to me, just one question about the tests.

@ElectreAAS ElectreAAS merged commit 1a0923e into ocaml:main Sep 12, 2025
10 of 12 checks passed
@ElectreAAS ElectreAAS deleted the console-print-rpc branch September 12, 2025 10:35
@Alizter
Copy link
Collaborator

Alizter commented Sep 12, 2025

The messages that were converted to paragraphs were not tested I believe. I'm fairly certain that putting a paragraph at the same level as Error: will cause it to break straight after, which is why we tend not to use paragraph in the first line of an error. Something like:

Error:
some paragraph

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.

3 participants