Skip to content

Conversation

@guillermoandrae
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Overwriting the parent method with a different signature is a bad idea IMO. You should use a different method name instead

@guillermoandrae
Copy link
Contributor Author

@stof thanks for the speedy and thorough review.

Copy link
Contributor

Choose a reason for hiding this comment

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

you should check that get is called with the hash in the headers too, not only with the second argument

@guillermoandrae
Copy link
Contributor Author

@stof I went ahead and added the remaining Enterprise API tests.

@guillermoandrae
Copy link
Contributor Author

@stof anything else I need to add here?

@guillermoandrae
Copy link
Contributor Author

Any word on this?

@stof
Copy link
Contributor

stof commented Nov 17, 2014

@guillermoandrae I'm not a maintainer on this repo. So it is not me merging this (I just provide review on the repo to help the maintainers)

@guillermoandrae
Copy link
Contributor Author

Got it, @stof. Thanks very much for the feedback!

Maybe @pilot or @cursedcoder ... can one of you guys take a look at this?

@pilot
Copy link
Contributor

pilot commented Dec 10, 2014

@guillermoandrae 👍

pilot added a commit that referenced this pull request Dec 10, 2014
Added ManagementConsole API and some missing tests.
@pilot pilot merged commit 40207d4 into KnpLabs:master Dec 10, 2014
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.

3 participants