Make lowercase columns default, add option to make them same as model #140
Conversation
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 |
|
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.' |
|
@lsetiawan See the conflicts that were identified by github. I'll hold off on reviewing this PR until the conflict is resolved. Thanks. |
| @@ -933,6 +933,8 @@ def getDataSetsValues(self, ids=None, codes=None, uuids=None, dstype=None): | |||
| uuids (list, optional): List of Dataset UUIDs string. | |||
| dstype (str, optional): Type of Dataset from | |||
| `controlled vocabulary name <http://vocabulary.odm2.org/datasettype/>`_. | |||
| lowercols (bool, optional): Make column names to be lowercase. | |||
| Default to True. | |||
There was a problem hiding this comment.
Add this at the end of the lowercols description: "Please start upgrading your code to rely on CamelCase column names. In a near-future release, the default will be changed to False, and later the parameter may be removed.
| @@ -1347,6 +1349,8 @@ def getResultValues(self, resultids, starttime=None, endtime=None): | |||
| resultids (list): List of SamplingFeatureIDs. | |||
| starttime (object, optional): Start time to filter by as datetime object. | |||
| endtime (object, optional): End time to filter by as datetime object. | |||
| lowercols (bool, optional): Make column names to be lowercase. | |||
| Default to True. | |||
There was a problem hiding this comment.
Add this at the end of the lowercols description: "Please start upgrading your code to rely on CamelCase column names. In a near-future release, the default will be changed to False, and later the parameter may be removed.
|
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! |
|
@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! |
|
@lsetiawan take a look at their status bot on Twitter. It seems that they had an incident a while ago: |
|
Oh okay. Good to know. I'll just let it do it's thing then. Thanks! |
|
Travis finally worked. Merging. |
Overview
This PR Addresses #127 (comment). Now
getResultValuesandgetDatasetsValueshave an additional argument calledlowercols, which defaults toTrueto 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
*valuesdataframe will default to columns similar to the model in future releases, therefore user have to addlowercase=Falseto anticipate for that?