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

Tiny refactoring #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

slawa85
Copy link

@slawa85 slawa85 commented Aug 22, 2017

This PR bring some small refactoring. The part with removed &block from method signature reduce a bit the code readability but increase the performance accordingly to this article https://mudge.name/2011/01/26/passing-blocks-in-ruby-without-block.html

@fabrik42
Copy link
Owner

fabrik42 commented Aug 23, 2017

Hi! Thank you for submitting this PR.

You added this commit:
648ca55

Why do you think the check is redundant?

Best,
Christian

@slawa85
Copy link
Author

slawa85 commented Aug 24, 2017

Hi,
As I saw, the meta_tags are not used at all in that sequence if code which is running once the condition is true, instead you are merging meta_tags here, that's why I considered it redundant there.

@fabrik42
Copy link
Owner

Hi,
first of all, sorry for answering so slowly.

It is correct that the meta hash is not used at all in this line.

It basically prepares the api response object to include the meta hash. The rule is, we don't want to mix the meta hash and the object that gets rendered. So we force a root node, if there is a meta hash. So this needs to stay, but there really should have been a test indicating that this is a required behaviour, so shame on me :)

Regarding the performance improvent:
This is something I didn't knew and I like it, thanks!

Best,
Christian

@slawa85
Copy link
Author

slawa85 commented Aug 28, 2017

Hi,

Thank you for explanation, now I got your idea and the approach that you use. I will edit my PR to include only the performance approach if you agree.

@fabrik42
Copy link
Owner

Sounds good! 👍

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.

2 participants