resolved issue #5 #6
Conversation
|
Sorry for adding so many commits. Will ensure to commit only when tested from my end. |
|
Thanks for the PR. Overall, all the code was written assuming parameters will be proper. So there is almost no error handling at all, which is bad in general. Sorry about that. Another big issue here is there is no proper tests/CI, which should be taken seriously when this grow bigger. |
|
@yunhailuo should I close this pr then? Thanks. |
There was a problem hiding this comment.
@arp95 Your PR is great. Please don't close it. Just continue to push commits to this PR/branch if you still want to work on this issue. My comments are just try to give some suggestions to hopefully improve this PR.
Your issues and PRs are meaningful, if I'm not clear before. Thank you! It is reasonable you use it, find bugs and fix it with PR. I have no problem with that and very much welcome it.
gdc.py
Outdated
| @@ -228,6 +228,7 @@ def search(endpoint, in_filter={}, exclude_filter={}, fields=[], expand=[], | |||
| response = requests.post(url, data=payload) | |||
| else: | |||
| response = requests.get(url, params=payload) | |||
|
|
|||
There was a problem hiding this comment.
Prefer not to have whitespace changes if not necessary.
There was a problem hiding this comment.
sure. will change that.
gdc.py
Outdated
| if(project_df.empty==False): | ||
| return project_df.set_index('id') | ||
| else: | ||
| return project_df |
There was a problem hiding this comment.
I think this function should just return project_df not matter what to make things consistent. And returning empty dataframe on failed/no-hit search seems reasonable to me. Therefore, things has to be changed here accordingly. And the check here may be whether project_df is None since search can return None. So check that and make sure it returns empty dataframe properly.
There was a problem hiding this comment.
@yunhailuo so you mean to return project_df itself? I mean the other functions would get changed where we were using project_df.set_index('id') state of dataframe.
There was a problem hiding this comment.
@yunhailuo Thanks for clearing out. Working on it.
There was a problem hiding this comment.
@yunhailuo if the project_df returned is null, then should I remove that project from projects array in gdc2xena file or just leave it empty?
Resolved error in gdc.py as stated in issue #5 .
Please check @yunhailuo