Skip to content
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

Fix Target attribute output #2306

Merged
merged 10 commits into from
Jun 26, 2020
Merged

Fix Target attribute output #2306

merged 10 commits into from
Jun 26, 2020

Conversation

innovate-invent
Copy link
Contributor

The current output does not contain enough information to describe an alignment. The current output is also not GFF3 compliant. This commit is in reference to #2301

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)

The current output does not contain enough information to describe an alignment. The current output is also not GFF3 compliant. This commit is in reference to #2301
@innovate-invent innovate-invent changed the title Fix Target attribute output [WIP] Fix Target attribute output Feb 20, 2019
@innovate-invent
Copy link
Contributor Author

@abretaud Thanks for the help. Sorry I forgot to set this to WIP. I have some pending commits that remove the code you are editing.

@innovate-invent innovate-invent changed the title [WIP] Fix Target attribute output Fix Target attribute output Feb 20, 2019
@innovate-invent
Copy link
Contributor Author

Testing the latest change exposes a bug in BioPy URI encoding '+' in the attributes when it shouldn't be.
I am going to abandon this tool for a simpler awk script that converts the backbone to GFF as I don't need scoring.

@abretaud
Copy link
Contributor

Is there an issue for this encoding problem in https://github.com/biopython/biopython?

@innovate-invent
Copy link
Contributor Author

Looks like the BCBio library has been depreciated.
https://github.com/chapmanb/bcbb/blob/master/gff/README.rst

@hexylena
Copy link
Member

@innovate-invent yeah it's deprecated the replacement for it has completely different semantics, and the current version of bcbio works sufficiently for most purposes. I tried to swap to gffutils at one point but had no success.

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

Looks good to me, not sure why %2B is being written rather than + though.

@innovate-invent
Copy link
Contributor Author

That is a standing bug in the aforementioned BCBio gff library.

@hexylena
Copy link
Member

hexylena commented Jun 23, 2020 via email

@innovate-invent
Copy link
Contributor Author

I believe this issue relates to the problem we are seeing: chapmanb/bcbb#86

@hexylena
Copy link
Member

@innovate-invent ok, I've added a small monkey patch for bcbio with the following logic:

  • scope:
    • very limited scope of what we're patching, already abstracted into a function for us.
    • tiny function to override
    • most importantly: we have significant control over what data is in the key values, we know it will be safe overall, so the more advanced things the original function does, does not matter for us.
  • bcbio is deprecated
    • I'm not interested in patching, other patches are upstreamed there but new releases are not forthcoming. the author has deprecated it, we should respect their wish to not be responsible for it, and forking is a significant undertaking.
    • I've had another look at the recommended replacement and it looks a lot more usable than it used to, so, switching to it is now a definite possibility
    • but dozens of tools depend on it, so, I'll do the migration to the new library in one go.

This patch has languished (my fault, I'm sorry) and it should definitely get in :) With this patch, I believe the tests will pass again.

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

Looks good, let's get this merged :) Thanks for the fix @innovate-invent!

@bgruening bgruening merged commit fc61c9d into galaxyproject:master Jun 26, 2020
@innovate-invent innovate-invent deleted the patch-1 branch June 26, 2020 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants