-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mz(X)MLMetadataCollector #280
base: master
Are you sure you want to change the base?
mz(X)MLMetadataCollector #280
Conversation
Hi, many adjustments but tool finally ready for a last checkup by the team before merging! Unfortunatelly test files have already been reduced to minimum and could not meet the size criteria.. |
<help><![CDATA[ | ||
.. class:: infomark | ||
|
||
**Credits** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the creators tag to provide credits: https://docs.galaxyproject.org/en/latest/dev/schema.html#tool-creator-person
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure how to specify the roles mentionned here using the creator tag. Would it be ok to simply add the creator tag as a complement for information about Quentin as the tool developer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just adding recommendations, you are free to ignore it :)
Do you miss anything in the creator tags that we should add? Those tags have been added to acknowledge tool developers and contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made a suggestion to add the tag in the XML (see dedicated comment).
The additional information I am unsure where to put using the creator tag is the role. I see there is a jobTitle tag, but I do not know whether it can be used to put this kind of information? As it is described in schema.org, it seems to be more about one's overall position, not really a specific role at a given scale. But I may very probably be wrong here as I am unfamiliar with these tags, actually. What I would have liked would have been to have an entry where I can put things like "original code developper" , "previous versions' participation", "maintainer", "interface supervision" for example. But again it may not be relevant, I can not gauge the usefullness in regard to the effort it would be to have an additional entry in this creator tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general comment, I think this script would be much more robust if it used a XML parser like lxml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid this is currently out of my scope of expertise to evaluate that (I have basic skills in Python but have no regular programming tasks to be fluent in this language). Since Quentin's temporary contract has come to term, he has no longer dedicated time to work on this. So I guess it will stay as it is for now, if it is not a limiting issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess your decision. It would happen that if you get slightly dirty XML with whitespace and such, that your hand-crafted parser will not work etc ... But that a theoretical problem, no clue if this is a practical concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the question of robustness to XML files' variations has been discussed previously with the users asking the tool to be shared, before we decide to wrap it. And they do acknowledge that in case the XML generation they performe may differ from the various-but-still-not-exhaustive use-cases with which the original tool has been used, it may lead to impossibility to correctly retreive the wanted information. Thus we added, when wrapping the tool, a comment in a "known issue" section of the tool help, for people to be aware of the limitation.
Hi @bgruening and thank you for the review. As @Hrodwin was in temporary contract position in my lab and the contract has come to term, I am in charge of finalizing his Galaxy tools' submission. So I have tried answering the comments, for as far as I can. In case there are limiting points I can not address relevantly I will contact him, but since he no longer has dedicated time to work on it I will try to handle all I can. |
correcting reference to support
</help> | ||
<citations> | ||
<citation type="doi">10.1093/bioinformatics/btu813</citation> | ||
</citations> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be ok as creator tags?
</citations> | |
</citations> | |
<creator> | |
<person givenName="Quentin" familyName="Ruin" jobTitle="Research engineer"></person> | |
<organization name="PFEM" url="https://eng-pfem.isc.inrae.fr/"></organization> | |
<organization name="MetaboHUB" url="https://www.metabohub.fr/home.html"></organization> | |
</creator> |
</description> | ||
<requirements> | ||
<requirement type="package" version="2.1.0">numpy</requirement> | ||
</requirements> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the pulsarification things, should we add this in the tool?
</requirements> | |
</requirements> | |
<required_files> | |
<include path="mzXMLMetadataCollector.py" /> | |
</required_files> |
FOR CONTRIBUTOR:
Hi, beginning the request for the deployment of a new tool gathering metadata from mzml and mzxml files: mz(X)MLMetadataCollector.