Mtgox Market implementation #15

Merged
merged 7 commits into from Jan 27, 2013

2 participants

@StevenLooman

Mtgox Market implementation. I'll continue on ActiveParticipant when the pull request is accepted.

Feel free to comment and/or ask.

@goncalopp
Owner

Solid work. I left a comment or two there, but it's mostly ready for merging

@StevenLooman

I removed the formatter by accident. Now that I'm thinking about it, the client-application of this library should specify the formatter I think. The Python logging framework provides a hierarchy in the loggers and the ability to configure the loggers in the hierarchy.
E.g., a client-application can specify which logger should print to console/write to file and how the log output should appear. I.e., using logging.getLogger(name) and user-configuration, the client-application can say: I want logging from mexbtcapi.market, but not from mexbtcapi.mtgox.

About the speed regarding value_int over value, I am currently not optimising anything but am following what I see in other files/places. We can further optimise when needed. At this stage, during development, I'm not worrying about speed.

About self._multiplier(BTC) vs self._multiplier(self.currency2), the first is easier to understand. self.currency2 should - in the case of MtGox - always be BTC as you can only trade a real currency against BTC. E.g., trading EUR to USD is not possible, but always goes 'through' BTC. self.currency1 is the variable part here, it can be EUR, or USD, or ... Its also a bit shorter and giving less hassle with regard to PEP8. If you really want me to, I can change it to self.currency2.

Thank you for your review and comments. Please let me know where you want to go.

@goncalopp
Owner

Agreed on leaving the optimization for later.

About self._multiplier(BTC), as long as mtgox doesn't suddenly allow inter-fiat-currency exchange, we're good to go. It it does, it's a couple of lines to change (for now), so we can go on with the show, I guess

About the logging: currently, there are no other loggers currently, so logging.getLogger("mexbtcapi.concepts.currency'") gets attached to the root logger.

the client-application of this library should specify the formatter I think

Ideally, the client application should be able to change the formatter (which is always true, since everything is public in python), but that doesn't mean we shouldn't provide sane defaults. Someone developing a new market shouldn't have to mess with logging to tell apart mexbtcapi's (core) messages from the market's.

Merging. I'll be stamping loggers everywhere, though, and seeing what can be done about the defaults

edit
it seems logging.basicConfig's formatter already prints the logger's name, so there's no need for a custom formatter, just for logging.basicConfig() on the hierarchy root.

@goncalopp goncalopp closed this Jan 27, 2013
@goncalopp goncalopp reopened this Jan 27, 2013
@goncalopp goncalopp merged commit 054d086 into goncalopp:master Jan 27, 2013
@StevenLooman

Great, thanks!

@StevenLooman StevenLooman deleted the unknown repository branch Feb 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment