Skip to content

Conversation

Hrodwin
Copy link

@Hrodwin Hrodwin commented Dec 22, 2024

FOR CONTRIBUTOR:

  • - I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • - License permits unrestricted use (educational + commercial)
  • - This PR adds a new tool or tool collection
  • - This PR updates an existing tool or tool collection
  • - This PR does something else (explain below)

Hi, beginning the request for the deployment of a new tool gathering metadata from mzml and mzxml files: mz(X)MLMetadataCollector.

@Hrodwin Hrodwin changed the title Adding tool mz(X)MLMetadataCollector Dec 22, 2024
@Hrodwin
Copy link
Author

Hrodwin commented Dec 23, 2024

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..

Copy link
Contributor

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

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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.

@melpetera
Copy link
Member

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.

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.

4 participants