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

Fixes #6768 - Hammer set-parameter does not work #1620

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@mbacovsky
Copy link
Member

commented Jul 27, 2014

Added scoped search to parameters. It helps hammer search parameters the same way as any other resource.

@dLobatog

This comment has been minimized.

Copy link
Member

commented Jul 28, 2014

@mbacovsky 👍

I tested it and it works well. My only concern is that all tests are exactly the same except for one word (host2, group2, etc..) and the new fixtures. Could you refactor them to avoid the code duplication and the new fixtures (by creating the parameters 'manually')?

@mbacovsky

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2014

I refactored the tests. @elobato, thanks for hints ;)

@dLobatog

This comment has been minimized.

Copy link
Member

commented Jul 29, 2014

Merged as 1eaa44c , thanks @mbacovsky !

@dLobatog dLobatog closed this Jul 29, 2014

@GregSutcliffe

This comment has been minimized.

Copy link
Member

commented Jul 29, 2014

@elobato can you ensure commit messages make sense before merging, please? This commit message implies we merged something that does not work :)

@mbacovsky

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2014

@GregSutcliffe it was supposed to mean that it fixes #6768, that has a subject "Hammer set-parameter does not work". Has it different sense written that way in English?

@GregSutcliffe

This comment has been minimized.

Copy link
Member

commented Jul 29, 2014

@mbacovsky, commit messages should accurately describe what functionality they add. The point is that the git logs should be readable, and a reader should be able to get a feel for what each commit does.

In this case, if in 6 months we had an issue with scoped search on parameters, and went looking for related commits, a reader might skip this one entirely, as it doesn't mention scoped search at all. I would expect to see something like "Fixes #6768 - Add scoped search to parameters model and controller".

It's not worth updating this one, since it's already been merged, but we're merging too many things with unclear/misleading messages at the moment, and yours was the first one I happened to see today :P

@mbacovsky

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2014

@GregSutcliffe, makes sense, thanks for explanation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.