hanami / router

Ruby/Rack HTTP router
http://hanamirb.org
MIT License
362 stars 92 forks source link

Refactored routes inspection output #211

Closed graudeejs closed 2 years ago

graudeejs commented 3 years ago

As discussed in https://github.com/hanami/router/pull/210 I've created 2nd Pull Request with refactored routes inspector output.

This one always writes output in columns. I tried it on my private project and I prefer this output better than in original Pull Requests. I suggest you try it on your own project to see how it looks (I'd love to share screenshot of my project, but since it's not public yet, I don't want to disclose any info about it).

jodosha commented 3 years ago

@graudeejs Thanks for this new PR, I need to pause this enhancement. I discovered that the inspector implementation doesn't work for hanami-api. I need to fix that first, and then get back to this.

jodosha commented 3 years ago

@graudeejs I fixed the compat problem with hanami-api. Would you please rebase unstable here and resolve the git conflicts? Thanks.

graudeejs commented 3 years ago

Rebased

graudeejs commented 3 years ago

I don't see how you could implement pretty printing routes without altering Hanami::Router::Inspector#call.

Perhaps route shouldn't be concerned with how to present itself to the user.

jodosha commented 3 years ago

@graudeejs Simply your enhancement should have a new place: the route. 🙂 Am I missing something?

graudeejs commented 3 years ago

@jodosha If I'm reading you correctly, then you're wrong.

Route has fields that may vary in length (path, to, constraints, name etc). A single route can ~now~ not know optimal padding for these fields in order to make consistent output (when inspector is outputing all routes. Route A and route B live in isolation, they don't check each other to figure out how to best present output).

For that reason route inspector calculates required padding for each field of all routes, to find max padding. Once max padding for each field is know you can print pretty output.

That's pretty much what this MR does.

Maybe I'm understanding Hanami::Router::Inspector wrong. From my perspective it is used to give user ability to print routes.

Alternatively I can create Hanami::Router::PrettyInspector, but I doubt that will be any better.

graudeejs commented 3 years ago

Where's what I want to achieve: router-inspec Consistent output.

P.S. I blurred out some stuff, as I don't want to reveal project name at this time.

graudeejs commented 3 years ago

@jodosha I'd love to hear from you. If you feel this MR is implemented in wrong way, then perhaps close it, I can reimplement this in my own code.

graudeejs commented 3 years ago

So I've added hack in my project to pretty print routes using router 2.0.0.alpha5

Had to monkey ~path~ patch Hanami::Router

# frozen_string_literal: true

require "hanami/router"

class Hanami::Router
  def to_inspect
     with(inspector: @inspector)
     @inspector.call
  end
end

This allows me to create my own inspector and inject it via existing router param inspector which is assigned to nil by default (I'm not sure that this is the best way, to do it, but it works for now)

It would be cool if inspect was assigned to new instance of Hanami::Router::Inspector and actually used in #to_inspect instead of crating new instance in #to_inspect method. This would allow to inject different inspectors potentially allowing to create inspector that prints json for example. That may be very useful in some cases.

Still would love to hear feedback from @jodosha, as current inspector fails to print good output in cases when fields are long.

jodosha commented 2 years ago

@graudeejs Thanks for your help. I'm closing this as stale. Thanks! 🙏

graudeejs commented 2 years ago

It's stale only, because you didn't reply :(

But thank you for your time nevertheless