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

getDataSets query mistake #117

Closed lsetiawan closed 6 years ago

lsetiawan commented 7 years ago

A mistake in getDataSets function is causing a failure when querying datasets by UUIDs.

https://github.com/ODM2/ODM2PythonAPI/blob/1010c1e88bf2b4cf71c67eb3bfd964e2169cb37e/odm2api/ODM2/services/readService.py#L687

emiliom commented 7 years ago

Yikes! It's surprising that's not a fatal bug (ie, the module should not work).

But it looks like an easy fix.

lsetiawan commented 7 years ago

Yepp, I have a fix

q = q.filter(DataSets.DataSetUUID.in_(uuids))

But, I am still debating whether to merge the big PR or not. I don't have a quick mechanism to test createServices, and there's not really much test for that. So I don't think I'll make further code changes until that's merged, might just wait for Stephanie. Meanwhile, I'll place my changes as issues.

emiliom commented 7 years ago

But, I am still debating whether to merge the big PR or not.

?? I gave the go-ahead on that, and I don't remember you expressing any objections. So what's changed? (we can discuss over email). I'm happy to merge myself right now :smiley_cat:

I don't have a quick mechanism to test createServices, and there's not really much test for that.

True, but it's obvious there's a bug, and the fix is obvious too. So I don't see any down side to submitting a PR to fix this now, while you're looking at it.

So I don't think I'll make further code changes until that's merged, might just wait for Stephanie.

I'll merge the big PR myself in about 15 minutes, unless you scream to tell me not to ...

lsetiawan commented 7 years ago

?? I gave the go-ahead on that, and I don't remember you expressing any objections. So what's changed? (we can discuss over email). I'm happy to merge myself right now

I just haven't tested other parts of the code and haven't tested the code thoroughly. I mean there's some tests in travis, but that's not everything, I see that a lot of the createService are skipped. So idk, I guess we will fix any bugs from this PR as it comes so far, for what I need it is working. And we can always see the previous changes.

True, but it's obvious there's a bug, and the fix is obvious too. So I don't see any down side to submitting a PR to fix this now, while you're looking at it.

Just don't want to give more rebasing complication. 😛

I'll merge the big PR myself in about 15 minutes, unless you scream to tell me not to ...

I am not stopping it. 👍

emiliom commented 7 years ago

Ok. I won't merge. Let's discuss on Monday morning and decide then whether to merge the big PR.

lsetiawan commented 7 years ago

Ok. I won't merge. Let's discuss on Monday morning and decide then whether to merge the big PR.

Alrighty, I'll take a look at the changes again, I haven't finished going through them all.

emiliom commented 7 years ago

Alrighty, I'll take a look at the changes again, I haven't finished going through them all.

Ok. But it can wait till Monday, if need be.

emiliom commented 7 years ago

@lsetiawan, if you have any time this week (probably not?), let me know if you feel we can move forward with the fix identified in this issue:

q = q.filter(DataSets.DataSetUUID.in_(uuids))

If you think we can move forward, I can prepare and submit the PR this week.

emiliom commented 6 years ago

@lsetiawan has this bug been fixed? If so, please close this issue. Otherwise, please go ahead and implement the fix (submit PR, which I'll review). It sounded like it was an easy and unobtrusive fix.

lsetiawan commented 6 years ago

Looks like it has been fixed: https://github.com/ODM2/ODM2PythonAPI/blob/development/odm2api/ODM2/services/readService.py#L794

emiliom commented 6 years ago

Thanks. Closing.