Conversation
|
@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? |
|
|
||
|
|
||
| class ReadODM2(serviceBase): | ||
| def _get_columns(self, model): |
|
|
||
| """ | ||
| type = self._session.query(Results).filter_by(ResultID=resultids[0]).first().ResultTypeCV | ||
| restype = self._session.query(Results).filter_by(ResultID=resultids[0]).first().ResultTypeCV |
There was a problem hiding this comment.
Thanks for using this opportunity to eliminate the use of reserved words (type), specially in a case like this one where it'll have no impact at all to code using this function.
There was a problem hiding this comment.
Actually, come to think of it, there's already a variable below called ResultType. restype and ResultType seem too similar, and that's confusing. How about renaming ResultType to ResultValues, assuming that doesn't lead to other conflicts or confusion?
There was a problem hiding this comment.
I needed to refresh to see this comment, I'll make that change. Thanks.
odm2api/ODM2/services/readService.py
Outdated
| con=self._session_factory.engine, | ||
| params=query.params | ||
| ) | ||
| df.columns = [self._get_columns(ResultType)[c] for c in df.columns] |
emiliom
left a comment
emiliom
left a comment
There was a problem hiding this comment.
Looks great, other than my suggested change regarding variable naming.
This is a PR to the dev branch that we have been merging into. This dev branch will then merge into master soon.
Unlesss I'm missing something, I don't see comment in regard to variable naming. |
|
@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? |
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 😜 |
|
Ok, I'll merge once the CI's are done. |
Yes, if a code is doing |
|
I'm assuming we can ignore the AppVeyor failures. So, going ahead and merging the PR. |
|
Awesome! Thanks. |
2 participants
Overview
This PR addresses #127. It changes the lowercase dataframe columns to the same cases as the model colump properties.