Skip to content

svengato intermine-ws-python enhancements#55

Merged
yochannah merged 4 commits intointermine:devfrom
svengato:dev
Oct 23, 2019
Merged

svengato intermine-ws-python enhancements#55
yochannah merged 4 commits intointermine:devfrom
svengato:dev

Conversation

@svengato
Copy link
Copy Markdown
Contributor

I am using the intermine python module to return templates from multiple InterMines. These changes are required to extract all the information I need.

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Oct 15, 2019

Hello @svengato! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-22 17:40:06 UTC

Copy link
Copy Markdown
Member

@yochannah yochannah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @svengato - thanks so much for making this PR! Adding the title and long description makes sense, but it might be handy if you could explain what exactly the other changes are intended to fix and how to test it. Once it is tested & verified okay we'd be happy to merge! :)

Comment thread intermine/query.py Outdated
@yochannah
Copy link
Copy Markdown
Member

yochannah commented Oct 15, 2019

p.s. - it is possible to fix the pep8 issues using the pep8 auto fix command in the readme. https://github.com/intermine/intermine-ws-python#before-making-prs

I also notice it's your first PR on GitHub - congrats! If you are thinking to make a few more (to InterMine or elsewhere) you coud sign up for hacktoberfest and get a free tshirt: https://hacktoberfest.digitalocean.com/

@yochannah
Copy link
Copy Markdown
Member

yochannah commented Oct 22, 2019

@svengato thanks for updating this to work nicely with PEP8! It looks like there are a couple of test failures - the new error text you wrote is being output in our logs: https://travis-ci.org/intermine/intermine-ws-python/jobs/598365963#L3101

Are you able to take a look?

@svengato
Copy link
Copy Markdown
Contributor Author

I just noticed that Template is a subclass of Query, so it seems more logical to handle parsing the template title there. Let me experiment with that and get back to you -

@svengato
Copy link
Copy Markdown
Contributor Author

It was a bit tricky as from_xml() is a Query class method, but this version should not generate the same error when parsing a non-template query.

Copy link
Copy Markdown
Member

@yochannah yochannah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now works when tested locally and also passes all tests - thanks @svengato !

@yochannah yochannah merged commit 0384fd0 into intermine:dev Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants