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

Some small fixes and enhancements #11

Closed
rjarry opened this issue Apr 11, 2012 · 1 comment
Closed

Some small fixes and enhancements #11

rjarry opened this issue Apr 11, 2012 · 1 comment

Comments

@rjarry
Copy link
Contributor

rjarry commented Apr 11, 2012

Hello ntt,

again thank you very much for your blazing fast reaction with our bug reports :)

In our project : Eve Corp. Management we use a slightly patched version of eveapi and I think that we made changes that could be interesting for you and others.

Here is a patch of our changes: http://pastebin.com/aEC5bJHX

Basically we changed the following:

  • Added an optional scheme argument (that defaults to "https" to the EVEAPIConnection function (to allow plain HTTP).
  • Fixed a typo on line 361: except Error, reason: --> except Error, e:
  • The _autocast function now converts dates to real datetime objects (easier to manage in Python than a unix timestamp).
  • Added a MalformedXMLResponse exception that fires instead of the RuntimeError if the <eveapi> tag is not found.
  • the version attribute of the <eveapi> tag is now available through a <api_connection_obj>._meta.version attribute.

Feel free to integrate those changes in the official version :)

Also, and most important, we wanted to know if you'd be interested in helping on Eve Corp. Management. We would be thrilled if you were :-)

Thanks again for a wonderful piece of software!!!

diab

@ntt
Copy link
Owner

ntt commented Apr 12, 2012

  • Hm, I'm not entirely sure on the usefulness of the scheme argument, considering it already selects the scheme based on the url's scheme already. The only benefit would be the ability to set the default in case no scheme was provided with the url.
  • nice catch on the typo.
  • autocast not returning a real Datetime object is mostly intentional. Not saying it's not a good idea, but I'm trying to keep things somewhat lightweight, and if I changed it now it'd cause a pretty major incompatibility with people's existing code. I'm trying to avoid such things.
    It's also perfectly fine to overload the _autocast function with your own if it suits your needs. I should probably add some function to provide an "official" way of doing that though.

On a side note, I noticed your code removed the integer check. There is a reason for that value.strip("-").isdigit() check; A raised exception is quite a few times more expensive than the cost of that check, and without it, it throws two exceptions per _autocast() if the value is not a number, instead of just one for the float() call.

  • version tag, uhm, it's pretty much going to be stuck at 2 for the rest of its useful life. With the upcoming CREST API I don't think this will be very relevant anymore. I will add the attribute as it doesn't hurt, but...
  • Your custom Exception, you're using a Warning for that and not an Error? I'm not about to let the parser continue on something that is for sure not what it's designed to parse. If you have a good reason to let it continue, I'd like to hear it.

Edit: and thanks for the invitation, but I have enough stuff on my TODO list as it is :)

@ntt ntt closed this as completed Nov 4, 2014
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

No branches or pull requests

2 participants