search

ODM2 / ODM2PythonAPI

A set of Python functions that provides data read/write access to an ODM2 database by leveraging SQLAlchemy.
http://odm2.github.io/ODM2PythonAPI/
BSD 3-Clause "New" or "Revised" License
4 stars 13 forks source link

Make lowercase columns default, add option to make them same as model #140

Closed lsetiawan closed 6 years ago

lsetiawan commented 6 years ago

Overview

This PR Addresses https://github.com/ODM2/ODM2PythonAPI/issues/127#issuecomment-356519409. Now getResultValues and getDatasetsValues have an additional argument called lowercols, which defaults to True to keep the api the same, and make changes less disruptive. I have tested this and it works.

I was wondering if I should create some sort of warning here too saying how this argument will be deprecated in the next release after this one, and the *values dataframe will default to columns similar to the model in future releases, therefore user have to add lowercase=False to anticipate for that?

emiliom commented 6 years ago

I was wondering if I should create some sort of warning here too saying how this argument will be deprecated in the next release after this one, and the *values dataframe will default to columns similar to the model in future releases, therefore user have to add lowercase=False to anticipate for that?

YES! That'd be perfect. Though at this time we don't need to be specific about which release it'll be deprecated on. We can say "soon" or "in the near future", then once a decision is made about a timeframe, we can change the warning to be more specific.

Regarding getDatasetsValues: I didn't realize that function was also impacted by this issue/decision. But I guess I hadn't really thought about or examined that function. Thanks for making the two functions consistent.

lsetiawan commented 6 years ago

Is this warning message okay?

'The parameter \'lowercols\' default makes the column names lowercase. '
 'Please set this to False. \'lowercols\' will be deprecated in the near future; '
 'column names casing will then be similar to its respective model.'
emiliom commented 6 years ago

@lsetiawan See the conflicts that were identified by github. I'll hold off on reviewing this PR until the conflict is resolved. Thanks.

emiliom commented 6 years ago

See my suggested changes to a couple of doc strings. If you agree with them, go ahead and merge this PR yourself once you've made that addition. Thanks!

lsetiawan commented 6 years ago

@ocefpaf For some reason, Travis CI is not starting, I've been waiting for it for few hours, I restarted it after about an hour. Do you know what's going on? Thanks!

ocefpaf commented 6 years ago

@lsetiawan take a look at their status bot on Twitter. It seems that they had an incident a while ago:

https://twitter.com/traviscistatus

lsetiawan commented 6 years ago

Oh okay. Good to know. I'll just let it do it's thing then. Thanks!

lsetiawan commented 6 years ago

Travis finally worked. Merging.