Skip to content
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

[10.x] Show CliDumper source content on last line #48707

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Oct 11, 2023

Hello!

Description

Please see #47514 for previous discussion.

This PR moves the CliDumper source content output from the first line to the last (non-empty) line.

Motivation

Showing the source output on the first line works for relatively small output (compared to the display height). However, a flaw is noticed as soon as the output significantly exceeds the display---a user is forced to scroll through all the lines just find the source or must resort to other means to track down the dump location in the codebase (such as redirecting stdout to a file and then viewing the head of the file). In essence, the convenience of locating the source with the content is lost for large outputs. Moving the source to the last line rectifies this and maintains the developer experience regardless of the dump size.

Screenshots

Before

image

image

image

After

image

image

image

Tag: @nunomaduro

Thanks!

@calebdw calebdw changed the title Show CliDumper source content on last line [10.x] Show CliDumper source content on last line Oct 11, 2023
@calebdw calebdw marked this pull request as draft October 11, 2023 17:46
@calebdw calebdw marked this pull request as ready for review October 11, 2023 18:32
@nunomaduro
Copy link
Member

nunomaduro commented Oct 12, 2023

@taylorotwell @driesvints While I don't agree with this change, I wanted to get your opinion on it. So, I've asked @calebdw to submit a PR individually, so we can also get your opinion on this.

Personally, I think while it does help solve the situation where the output of a dd can be big, it does make the output a little bit worse for the most common usage of the dd: small outputs.

Note: coding-wise, the pull request is OK.

Screenshot 2023-10-12 at 09 09 42

@nunomaduro nunomaduro requested review from taylorotwell and driesvints and removed request for nunomaduro October 12, 2023 08:13
@driesvints
Copy link
Member

I think the before is fine really.

@taylorotwell taylorotwell merged commit 56beb0e into laravel:10.x Oct 12, 2023
21 checks passed
@taylorotwell
Copy link
Member

I think I like this overall.

@calebdw calebdw deleted the cli_dd branch October 12, 2023 13:37
timacdonald pushed a commit to timacdonald/framework that referenced this pull request Oct 24, 2023
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