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

Add Colorized, a new routes formatter #45241

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

faqndo97
Copy link
Contributor

@faqndo97 faqndo97 commented Jun 1, 2022

Summary

I added ActionDispatch::Routing::ConsoleFormatter::Colorized that is a new routes formatter.

Why a new formatter?

The intention of this one is to add a different structure and colors to the actual rails route command.
This formatter can be used by a new flag (-C) added on the route command.

Before

Screen Shot 2022-06-01 at 6 10 17 PM

After

Screen Shot 2022-06-01 at 6 10 02 PM

I added `ActionDispatch::Routing::ConsoleFormatter::Colorized` that is a new routes formatter.

Why a new formatter?

The intention  of this one is to add a different structure and colors to the actual `rails route` command.
This formatter can be used by a new flag (`-C`) added on the `route` command.
end

def colorize_path(path)
path.gsub(/\/(:[a-z]*)/, "/#{YELLOW}\\1#{CLEAR}")
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't fully cover a param. For example, your screenshot only highlights :inbound of :inbound_email_id. Parameters can also include numbers, underscores, capitals, and possibly other characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, thanks! I was trying some ways to solve this, what do you think about this?

path.gsub(/\/(:[\w\d]*)/, "/#{YELLOW}\\1#{CLEAR}")

Here before and after that change:

image

@Diogomartf
Copy link

Diogomartf commented Jun 2, 2022

I would love to see this merged 🙌 Such a nice detail. ❤️

def name_and_reqs(route)
return route[:reqs] if route[:name].blank?

"#{route[:name]}_path > #{route[:reqs]}"

Choose a reason for hiding this comment

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

Just a suggestion: I would swap > with ->, I think it's more clear.

Suggested change
"#{route[:name]}_path > #{route[:reqs]}"
"#{route[:name]}_path -> #{route[:reqs]}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the suggestion but maybe we can use a Unicode character to represent the arrow, something like \u279C that prints

Choose a reason for hiding this comment

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

Yeah, that would be even better.

@alessandrapereyra
Copy link

Shared this on Twitter as well, but I think it'd be cool if we could separate PUT/PATCH from the actual POST requests for ease of readability.

This is already doing it with DELETE, so it does make sense to me to have it as different categories.

Great addition, it makes things better to easily parse 🎉 .

@natematykiewicz
Copy link
Contributor

natematykiewicz commented Jun 2, 2022

Shared this on Twitter as well, but I think it'd be cool if we could separate PUT/PATCH from the actual POST requests for ease of readability.

This is already doing it with DELETE, so it does make sense to me to have it as different categories.

Great addition, it makes things better to easily parse 🎉 .

Yeah, I've commonly seen orange used for the PUT/PATCH in other REST clients.
For example: https://swagger.io/tools/swagger-ui/

@faqndo97 faqndo97 changed the title feat: Add Colorized, a new routes formatter Add Colorized, a new routes formatter Jun 2, 2022
@skipkayhil
Copy link
Member

I think instead of --colorized I would love to see a more general --format option, and an API to register formatters. Then applications/gems could define new formatters and it wouldn't require patches to Rails to use them. I think this would also prevent unexpected results if someone were to pass both --expanded and --colorized, since expanded is checked first in the formatter method.

@natematykiewicz
Copy link
Contributor

natematykiewicz commented Jun 2, 2022

@skipkayhil By implementing what you're saying, --expanded --colorized would actually colorize the "expanded" format, right?

The current --colorized really looks like both "colorized" AND "justified".

So, is "colorized" an option, but then "sheet", "expanded" and "justified" are formats? Perhaps these screenshots should actually be --colorized --justified?

@natematykiewicz
Copy link
Contributor

natematykiewicz commented Jun 2, 2022

FWIW, most CLI things I've seen that have a "colorized" option do not change any other formatting. They just add color codes when you colorize the output. Having a colorized flag that also rearranges the output is a bit nonstandard. That's why I'm thinking that "colorized" should apply to all formatters, and then if you want a new formatter for this justified output, that makes sense to me too.

@skipkayhil
Copy link
Member

@skipkayhil By implementing what you're saying, --expanded --colorized would actually colorize the "expanded" format, right?

The current --colorized really looks like both "colorized" AND "justified".

So, is "colorized" an option, but then "sheet", "expanded" and "justified" are formats? Perhaps these screenshots should actually be --colorized --justified?

Yep, that all seems reasonable to me. If the implementation was changed to just colorize the existing format I think that would provide justification for a --colorized flag.

@matthewd
Copy link
Member

matthewd commented Jun 5, 2022

Can you perhaps expand on what's better about this layout, colours aside?

At first look, to me, it seems to put all the emphasis on the path patterns, and makes it hard to match them up with their corresponding helpers. Most of the time I would expect users to want the opposite: for most purposes, one should be thinking in and using path helpers, and only occasionally need to deal with exactly which URLs they generate/match.

@faqndo97
Copy link
Contributor Author

faqndo97 commented Jun 8, 2022

Ok, I have been thinking about how to continue with this based on all the feedback received here and on Twitter.

@matthewd I think this new layout is more natural to find what we're looking for in the output. From my experience, when we run the command, most of the time, the information we have is the path, and we're looking for the helper or controller/action or both. So, how we read from left to right, display the information we tend to have on the left side, and put the information we're looking for on the right side matches that. Again, this is from my POV and experience.

Related to what @skipkayhil and @natematykiewicz were talking about, I agree that maybe the "colorize" concept should be a modifier of all formatters and not a formatter itself.

I didn't want to jump directly and replace the default one (Sheet) because probably some people will continue using it.

So maybe the best approach to continue with this, as @natematykiewicz proposed, is to rename this formatter to Justified and refactor the remind formatters to accept the colorized modifier. What do you think?

Related to other comments for improvements that I received, the notes I took are:

  • Use a different color for post.
  • Use a grayish color for (.:format).
  • Use a different color for the path helper instead of gray to highlight it more.
  • Improve the separator between helper name and formatter.

@phendrick
Copy link

Hey @faqndo97 this looks awesome! As mentioned above, i've opened #45330 which allows for registering custom formatters; maybe there's a combination of these two PR's we can work on?

@Diogomartf
Copy link

Is this branch still alive?

Would love to see this feature shipped. 🚀

Happy to help.

@Diogomartf
Copy link

would be great to see this PR merged on the main branch, what does it take to get there? Or there is no interest on the community to have this merged?

Either way would be good to know where we stand.

I think rails routes command needs a lift up and this is a step in the right direction.

@faqndo97
Copy link
Contributor Author

@Diogomartf Thanks for being interested on this. I couldn't come back here for some time, but I'll think what can be the best next steps here and continue working + communicating next week hopefully

@dhh
Copy link
Member

dhh commented Jan 1, 2024

Really dig this! So much so that I think it should be the default and we use the flag to turn it off, when needed.

@simi
Copy link
Contributor

simi commented Jan 1, 2024

Any reason to not use https://github.com/fazibear/colorize logic instead of defining this inline in routing inspector? Alternatively would it make sense to make "colorize" functionality into Active Support and use in here? IMHO it could be used for more outputs later.

@dhh
Copy link
Member

dhh commented Jan 1, 2024

This doesn't warrant another gem dependency. If we can make it happen inline, great, otherwise, no.

@faqndo97
Copy link
Contributor Author

faqndo97 commented Jan 2, 2024

@dhh Thanks for the feedback, will solve conflicts, and make the changes this week.

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.

None yet

9 participants