Skip to content

fix(grape): fix deprecation warning#13

Open
raszi wants to merge 2 commits intojondot:masterfrom
raszi:fix_gradle
Open

fix(grape): fix deprecation warning#13
raszi wants to merge 2 commits intojondot:masterfrom
raszi:fix_gradle

Conversation

@raszi
Copy link

@raszi raszi commented Nov 18, 2016

No description provided.

@coveralls
Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.006%) to 97.706% when pulling eede199 on raszi:fix_gradle into 66e1c8e on jondot:master.

@coveralls
Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.006%) to 97.706% when pulling 5063b0d on raszi:fix_gradle into 66e1c8e on jondot:master.

@jondot
Copy link
Owner

jondot commented Nov 18, 2016

Thanks! I'm guessing you mean Grape and not Gradle?
Small suggestion - can we cache places where respond_to is used in the hot path? (so not to affect performance)

@raszi raszi changed the title fix(gradle): fix deprecation warning fix(grape): fix deprecation warning Nov 19, 2016
@raszi
Copy link
Author

raszi commented Nov 19, 2016

Yes, I am sorry, it was late when I opened the pull-request yesterday. :)

@raszi
Copy link
Author

raszi commented Nov 19, 2016

Sure, I'll adjust my PR and I'll do a force push to fix the git commit messages.

@raszi
Copy link
Author

raszi commented Nov 19, 2016

I just verified whether it is necessary or not:

Warming up --------------------------------------
         respond_to?   216.089k i/100ms
              cached   261.694k i/100ms
Calculating -------------------------------------
         respond_to?      5.608M (± 5.8%) i/s -     28.092M in   5.026076s
              cached      8.979M (± 7.9%) i/s -     44.750M in   5.018407s

Comparison:
              cached:  8978741.0 i/s
         respond_to?:  5608259.8 i/s - 1.60x  slower

@coveralls
Copy link

coveralls commented Nov 19, 2016

Coverage Status

Coverage increased (+0.006%) to 97.706% when pulling 67905db on raszi:fix_gradle into 66e1c8e on jondot:master.

@raszi
Copy link
Author

raszi commented Nov 21, 2016

@jondot sorry, but with the current implementation I can see no good place to put the caching in place.

If you have any recommendations I am open for that.

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