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 option for display header text for routes inspector #78

Merged
merged 1 commit into from
Dec 20, 2015

Conversation

davydovanton
Copy link
Member

Why this is needful

This option will provide better formatting for lotus routes command, for example:

~/work/lotus_server » lotus routes
                Name Method   Path                      Action
                     POST     /api/action               Api::Controllers::Files::Action

/cc @jodosha

@jodosha
Copy link
Member

jodosha commented Dec 19, 2015

@davydovanton This is good for clarity 👍

I'd add a line of separation between the headers and the routes.

methods: 'Method'.freeze,
path: 'Path'.freeze,
endpoint: 'Action'.freeze
]
Copy link
Member

Choose a reason for hiding this comment

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

@davydovanton Can you please freeze the hash instead of the single values?

Copy link
Member Author

Choose a reason for hiding this comment

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

nice idea, I like this 👍

@davydovanton
Copy link
Member Author

@jodosha done. If you want I can squash the commits to one

@runlevel5
Copy link
Member

nice one

@davydovanton
Copy link
Member Author

thanks @joneslee85! 😊

@jodosha
Copy link
Member

jodosha commented Dec 20, 2015

Please squash them!

http://lucaguidi.com

On 19/dic/2015, at 21:42, Anton Davydov notifications@github.com wrote:

@jodosha done. If you want I can squash the commits to one


Reply to this email directly or view it on GitHub.

@davydovanton
Copy link
Member Author

@jodosha done 🎉

@jodosha jodosha self-assigned this Dec 20, 2015
@jodosha jodosha added this to the v0.5.0 milestone Dec 20, 2015
@jodosha
Copy link
Member

jodosha commented Dec 20, 2015

@davydovanton ✌️

jodosha added a commit that referenced this pull request Dec 20, 2015
Add option for display header text for routes inspector
@jodosha jodosha merged commit 39b3337 into hanami:master Dec 20, 2015
@davydovanton davydovanton deleted the display-routes-header branch December 20, 2015 10:27
@davydovanton
Copy link
Member Author

Thanks! 🌟

P.S.: @jodosha I want to add search pattern (like in rails/rails#18902) what do you think it's good idea?

@jodosha
Copy link
Member

jodosha commented Dec 20, 2015

@davydovanton No thanks 😄

When in doubt, always rely on UNIX.

@davydovanton
Copy link
Member Author

:trollface:

@jodosha
Copy link
Member

jodosha commented Dec 20, 2015

@davydovanton No, I wasn't trolling. Sorry if you've got that feeling. Let me better explain.

Our inspector relies on Ruby's way to print on stdout. That is compliant with UNIX pipes. So we don't have to worry how the output of our command is used by other programs like grep.

If we include that -g option in our command. We need to take care that it works similarly to grep and that the output is compliant with UNIX pipes. This increases the maintenance load for us. Think of potential bugs because we don't respect UNIX standards.

Because grep is around way before I was born, and it works great, I don't see the reason to reproduce it.

The less features you add, the less you have to worry about.

@davydovanton
Copy link
Member Author

wow, thanks for explanation! I agree with you and I think the Unix way is missing in rails.

@jodosha
Copy link
Member

jodosha commented Dec 20, 2015

A further clarification on what stated above. It's possible to make this feature fully compliant with UNIX. That -g <routename> can be used to filter the routes internally and only to print those who are matching the input.

Pseudo code:

routes = ['books', 'book']
input   = 'books'

result = routes.find_all do |route|
  route.match /#{ input }/
end

puts result

However, grep already does that.

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

3 participants