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

Avoid duplicated calls in StackableValue#[] #1561

Conversation

@brucehsu
Copy link
Contributor

@brucehsu brucehsu commented Jan 26, 2017

Fixing the performance issue mentioned in issue #1560 by eliminating duplicated calls of StackableValue#[] in one-liner conditionals.

@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 99.071% when pulling 061cf11 on brucehsu:feature/fix_excessive_recursion_in_stackablevalue into 3b91395 on ruby-grape:master.

3 similar comments
@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 99.071% when pulling 061cf11 on brucehsu:feature/fix_excessive_recursion_in_stackablevalue into 3b91395 on ruby-grape:master.

@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 99.071% when pulling 061cf11 on brucehsu:feature/fix_excessive_recursion_in_stackablevalue into 3b91395 on ruby-grape:master.

@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 99.071% when pulling 061cf11 on brucehsu:feature/fix_excessive_recursion_in_stackablevalue into 3b91395 on ruby-grape:master.

@brucehsu
Copy link
Contributor Author

@brucehsu brucehsu commented Jan 26, 2017

Travis-CI builds are failing on Ruby 2.3.3 and Ruby 2.4, due to this issue in Rainbow 2.2.1 : sickill/rainbow#48

@brucehsu
Copy link
Contributor Author

@brucehsu brucehsu commented Jan 26, 2017

Please also refer to: travis-ci/travis-ci#7204

@brucehsu
Copy link
Contributor Author

@brucehsu brucehsu commented Jan 26, 2017

Fixed in PR #1562 .

@dblock
Copy link
Member

@dblock dblock commented Jan 26, 2017

This makes a lot of sense. Rebase (I merged #1562) please, and add a CHANGELOG line in line with something like "Improved performance of ..." so that it becomes a visible change.

@brucehsu brucehsu force-pushed the brucehsu:feature/fix_excessive_recursion_in_stackablevalue branch from 061cf11 to 882eb65 Jan 26, 2017
@grape-bot
Copy link

@grape-bot grape-bot commented Jan 26, 2017

1 Warning
⚠️ There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 99.071% when pulling 882eb65 on brucehsu:feature/fix_excessive_recursion_in_stackablevalue into b4858f3 on ruby-grape:master.

2 similar comments
@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 99.071% when pulling 882eb65 on brucehsu:feature/fix_excessive_recursion_in_stackablevalue into b4858f3 on ruby-grape:master.

@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2017

Coverage Status

Coverage remained the same at 99.071% when pulling 882eb65 on brucehsu:feature/fix_excessive_recursion_in_stackablevalue into b4858f3 on ruby-grape:master.

@dblock dblock merged commit e1a14bf into ruby-grape:master Jan 26, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dblock
Copy link
Member

@dblock dblock commented Jan 26, 2017

Thanks @brucehsu. I'll make a minor release sometime soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.