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

refactor get_top_markets issue #842 #1549

Merged
merged 2 commits into from Feb 5, 2019

Conversation

Projects
4 participants
@oxarbitrage
Copy link
Member

commented Jan 31, 2019

This is a replacement for pull request #1273 for issue #842.

Sorry i had to do it this way, the reason of the replacement is build problems at oxarbitrage:issue842 that i was not able to fix by merging the develop into the feature branch. Some issue with the websocket submodule.
Anyway, to avoid losing too much time on it i created this one that is almost the same(the last review comments were already addressed in #1273). On top of that, new pull modifies the method names according to our (not written yet) style guide: #1318 (comment)

@jmjatlanta
Copy link
Contributor

left a comment

Some minor changes in the constructor may allow for in-place construction.

price latest_price = asset( mto.latest_base, mto.base ) / asset( mto.latest_quote, mto.quote );
if( mto.base != asset_base.id )
latest_price = ~latest_price;
latest = database_api_impl::price_to_string( latest_price, asset_base, asset_quote );

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jan 31, 2019

Contributor

Refactor suggestion (may be out of the scope of this ticket): put database_api_impl::price_to_string in some kind of common area. market_ticker should not have to know about database_api_impl, and database_api_impl::price_to_string shouldn't know about graphene::app::price_to_string.

Show resolved Hide resolved libraries/app/database_api.cpp Outdated
Show resolved Hide resolved libraries/app/database_api.cpp Outdated
Show resolved Hide resolved libraries/app/database_api.cpp Outdated
@abitmore

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

@jmjatlanta you may have interest in the first PR (#924) where we've addressed some issues. It's a pity that the code was abandoned.

@ryanRfox ryanRfox added this to In progress in Feature Release (201902) via automation Feb 1, 2019

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

I had refactored to in-place constructor by comments from @jmjatlanta and by using parts of #924

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

@abitmore please have a look for Review of this PR. Thanks

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@jmjatlanta you may have interest in the first PR (#924) where we've addressed some issues. It's a pity that the code was abandoned.

Thanks. I took a look at it. This code looks very similar now. Is there something that you liked better in #924? Or is it just the fact that it was done twice?

Feature Release (201902) automation moved this from In progress to In Review Feb 5, 2019

@oxarbitrage oxarbitrage merged commit c539336 into bitshares:develop Feb 5, 2019

1 of 2 checks passed

ci/dockercloud Your tests failed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature Release (201902) automation moved this from In Review to Done Feb 5, 2019

@abitmore abitmore added this to the 201902 - Feature Release milestone Feb 5, 2019

@Zapata Zapata referenced this pull request Feb 27, 2019

Closed

Empty 'percent_change' in get_ticker API #1620

1 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.