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

Roar decorator support #10

Closed
wants to merge 11 commits into from

Conversation

sdbondi
Copy link
Contributor

@sdbondi sdbondi commented Dec 17, 2014

Added Grape::Roar::Decorator to allow the decorator pattern to be used in grape.
Please see updated README for details.

Thanks!

end
```

Note this issue when using decorators: https://github.com/apotonick/roar/issues/92
Copy link
Member

Choose a reason for hiding this comment

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

I am not 100% sure this is necessary, but if you think it is, I would describe it here fully and then provide a link.

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 was thinking that it is pretty helpful and will hopefully save people having to look up why their code isn't working/save an issue on this repo. Having said that, it would be better if it was documented on roar itself. I'll leave that out for now.

@dblock
Copy link
Member

dblock commented Dec 17, 2014

This is great, thanks! Needs a CHANGELOG entry and a build fix (Rubocop), please. Also would appreciate it if you could squash the commits.

@sdbondi
Copy link
Contributor Author

sdbondi commented Dec 18, 2014

Before I squash, is everything 100%? version 0.1.1, CHANGELOG entry etc.
One question about squashing, do I create another pull request for it or can I attach it to this PR from a new rebased branch?

@@ -1,3 +1,8 @@
0.1.1 (18/12/2014)
Copy link
Member

Choose a reason for hiding this comment

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

This should have a TBD date, otherwise people think it's released. I usually don't put a version in here either, so would just say Next.

@dblock
Copy link
Member

dblock commented Dec 18, 2014

I added some very nitpicky comments. Thanks for your patience.

@dblock
Copy link
Member

dblock commented Dec 18, 2014

I can't stop thinking about how we misspell Roar now and how it should be ROAR being an acronym:) j/k, but cc: @apotonick

@sdbondi
Copy link
Contributor Author

sdbondi commented Dec 18, 2014

Haha, OCD is a b%$#h - nitpicking is no problem glad I could help :)

@dblock
Copy link
Member

dblock commented Dec 18, 2014

This is good, squash and I'll merge.

@sdbondi
Copy link
Contributor Author

sdbondi commented Dec 18, 2014

Closing in favour of #11

@sdbondi sdbondi closed this Dec 18, 2014
@apotonick
Copy link

I gave a talk What is Roar and why does Katy Perry sing about it? and now she wants to marry me: https://twitter.com/apotonick/status/421250134140137472

@dblock
Copy link
Member

dblock commented Dec 19, 2014

@apotonick Katy sends you her love.

@apotonick
Copy link

Thanks @dblock. Kisses back 💋

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