Add linter for DOI type citation.#484
Conversation
|
Very nice - I love this! |
|
Thanks @jmchilton - seems like I created a false positive in the tests though |
| @@ -0,0 +1,31 @@ | |||
| <tool id="copy" name="Copy Dataset" version="1.0"> | |||
| <description>copies a dataset</description> | |||
There was a problem hiding this comment.
The test framework just ensures all fail_ tools fail linting - just rename to invaild_doi or something and the failing test will work.
There was a problem hiding this comment.
Ahh, indeed, that did the trick, thanks!
Improve parsing of doi citation tag from xml, use lint_ctx.warn in case of unexpected response from dx.doi and uppercase Galaxy (thanks @nsoranzo).
tests/test_lint.py
Outdated
| self._check_exit_code(lint_cmd, exit_code=0) | ||
|
|
||
| def test_lint_doi(self): | ||
| fail_doi = os.path.join(TEST_TOOLS_DIR, "invalid.xml_doi.xml") |
There was a problem hiding this comment.
s/invalid.xml_doi.xml/invalid_doi.xml/ ?
|
Awesome thanks! |
| r = requests.get(url) | ||
| if r.status_code == 200: | ||
| if publication_id != doiless_publication_id: | ||
| lint_ctx.error("%s is valid, but Galaxy expects DOI without 'doi:' prefix" % publication_id) |
There was a problem hiding this comment.
Are you sure about this? @bgruening pointed out in galaxyproject/tools-iuc#768 (comment) that the initial 'doi:' seems to work fine.
There was a problem hiding this comment.
I just tested it again, the doi breaks the display of the reference with my terribly out-of-date planemo instance (September 2015 I believe), but newer galaxy can deal with the doi:. I guess we could tone this down to an info?
There was a problem hiding this comment.
Something like:
lint_ctx.warn("%s is valid, but old Galaxy versions expected DOI without 'doi:' prefix" % publication_id)?
There was a problem hiding this comment.
Yes ... but given those old instances will disappear over time, maybe we can remove the warning altogether? Alternatively we should at least figure out how old is old :/
There was a problem hiding this comment.
Up to you or @jmchilton, I don't have a strong opinion on this.
There was a problem hiding this comment.
I have no clue what changed to enable those - I just rewalked through the code and I can't even figure out why it works at all with the leading doi: - I don't think it should. Anyway - I definitely consider a leading doi: a tool bug and on a purely stylistic point somewhat redundant given the citation type is "doi". I think warn is entirely appropriate here.
I guess this is useful on its own, and may prove more valuable if we support plugging tool information from http://bio.tools into
planemo tool_init.