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

[9.x] Prettify the event:list command #41977

Closed
wants to merge 2 commits into from

Conversation

imanghafoori1
Copy link
Contributor

@imanghafoori1 imanghafoori1 commented Apr 14, 2022

The current form of printing a table for showing events/listeners is very sensitive to the width of the command line. In this form, we can survive much smaller screens.

  • It also indicates which listener implements the ShouldQueue and ShouldBroadcast interface.

image

The new output is responsive to the width of the terminal window and trims the long lines.

image

You may change the text and color to your liking.


  • This is the before, which has problems when an event has more than one listener:

image

@imanghafoori1 imanghafoori1 force-pushed the event_list branch 11 times, most recently from 6273ac0 to 8bf6e59 Compare April 17, 2022 16:16
@GrahamCampbell
Copy link
Member

This is a breaking change, no?

@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Apr 17, 2022

  • Yeah, to some level, because the protected getEvents method may be used in a subclass, but that is very unlikely, plus the fact that the command is only used locally and not in production.
  • We can change it to be fully backward compatible if needed.

@imanghafoori1 imanghafoori1 force-pushed the event_list branch 2 times, most recently from 50b851d to 54dc203 Compare April 17, 2022 17:38
@GrahamCampbell
Copy link
Member

I also meant the fact that the output was changing.

@driesvints
Copy link
Member

@GrahamCampbell I personally don't consider that part of the public API but let's see what Taylor says.

@taylorotwell
Copy link
Member

Personally I'd like to see the formatting be a bit more in line with route:list in terms of how listeners are displayed below the events, etc. so it feels consistent. Maybe @xiCO2k would be willing to help?

@xiCO2k
Copy link
Contributor

xiCO2k commented Apr 19, 2022

Actually I did start a PR for that, will find the time to finish it

@xiCO2k
Copy link
Contributor

xiCO2k commented Apr 19, 2022

Also @imanghafoori1 let me know if you want to match the look with route:list on this PR.

@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Apr 20, 2022 via email

@taylorotwell taylorotwell marked this pull request as draft April 20, 2022 14:21
@xiCO2k
Copy link
Contributor

xiCO2k commented Apr 20, 2022

Awesome @imanghafoori1, if you need any help, let me know ;)

@imanghafoori1
Copy link
Contributor Author

imanghafoori1 commented Apr 20, 2022

@xiCO2k Thank you very much, currently working on it, and I will push the results in an hour or so.

@imanghafoori1
Copy link
Contributor Author

With the push of the second commit, I am done. \(^_^)/

@imanghafoori1 imanghafoori1 marked this pull request as ready for review April 20, 2022 16:41
@taylorotwell
Copy link
Member

@xiCO2k can you share what your version looked like?

@xiCO2k
Copy link
Contributor

xiCO2k commented Apr 20, 2022

I was trying 3 different looks

First:
Screen Shot 2022-04-20 at 19 40 24

Second:
Screen Shot 2022-04-20 at 19 44 55

Third:
Screen Shot 2022-04-20 at 19 47 23

I think what fits the best is the first one, but not sure how it should look when there is more than one listener.

Let me know what you guys think.

@taylorotwell
Copy link
Member

taylorotwell commented Apr 20, 2022

Personally I like the third one best... with an empty line added in between new events to give it a bit more breathing room. 👍

Simple and clean. 🧼

@xiCO2k
Copy link
Contributor

xiCO2k commented Apr 20, 2022

I can PR that style change, and then @imanghafoori1 after the merge can PR those cool additions to show ShouldQueue and stuff like that.

What do you guys think?
@taylorotwell @imanghafoori1

@imanghafoori1
Copy link
Contributor Author

@xiCO2k I have no idea, actually.

@taylorotwell
Copy link
Member

@xiCO2k sure go ahead and send over your style change.

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.

5 participants