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

Fix getresultvalues #136

Closed lsetiawan closed 6 years ago

lsetiawan commented 6 years ago

Overview

This PR addresses #127. It changes the lowercase dataframe columns to the same cases as the model colump properties.

emiliom commented 6 years ago

@lsetiawan I have a broad question: this is a PR on master. But there is a bunch of commits on a dev branch that @Elijahwalkerwest will merge into master soon, hopefully early next week, right?

Given that, is it better to wait until Eli has merged his accumulated commits into master? Or are you ok with merging this PR now and possibly needing to spend time resolving conflicts later?

lsetiawan commented 6 years ago

@lsetiawan I have a broad question: this is a PR on master. But there is a bunch of commits on a dev branch that @Elijahwalkerwest will merge into master soon, hopefully early next week, right?

This is a PR to the dev branch that we have been merging into. This dev branch will then merge into master soon.

my suggested change regarding variable naming

Unlesss I'm missing something, I don't see comment in regard to variable naming.

emiliom commented 6 years ago

@lsetiawan I'll hold off on approving and merging, until I hear back from you about my broader comment plus my suggestion to rename a variable. But nice job!

Also, let's make sure Jeff et al are very aware of this change. Correct me if I'm wrong, but all existing code that uses output from this function will need to be upgraded to the case-sensitive dataframe column names, right?

emiliom commented 6 years ago

This is a PR to the dev branch that we have been merging into. This dev branch will then merge into master soon.

Great. My bad! I thought I checked for that, but obviously I didn't! My excuse is that this is my first PR review in a month, and ... also ... this year :stuck_out_tongue_winking_eye:

emiliom commented 6 years ago

Ok, I'll merge once the CI's are done.

lsetiawan commented 6 years ago

Correct me if I'm wrong, but all existing code that uses output from this function will need to be upgraded to the case-sensitive dataframe column names, right?

Yes, if a code is doing df['datavalue'] for example, it won't work anymore.

emiliom commented 6 years ago

I'm assuming we can ignore the AppVeyor failures. So, going ahead and merging the PR.

lsetiawan commented 6 years ago

Awesome! Thanks.