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

🔨 Scrape Number and Specify Description #34

Merged
merged 11 commits into from
Jan 31, 2024
Merged

Conversation

migbro
Copy link
Contributor

@migbro migbro commented Jan 26, 2024

Partly addresses feature request # #33

  • Is able to parse and return the correct Number attribute for info to update_header
  • Allows user to specify in config a Description to override the default
    TO DO: Actually scrape the description from the source vcf file

Built locally, got compiler warning:

warning: unreachable pattern
   --> src/commands/encoder_cmd.rs:226:13
    |
226 |             _ => panic!(
    |             ^
    |
    = note: `#[warn(unreachable_patterns)]` on by default

When run, headers are output as desired

Copy link
Owner

@brentp brentp left a comment

Choose a reason for hiding this comment

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

Thanks very much! Looks good.
Can you also add a test?

src/lib/echtvar.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
🚧 test in progress
tests/big.sh Outdated
@@ -61,7 +61,7 @@ for mod in 2 3 4 5; do


done
rm generated-all.vcf generated-subset0.vcf anno.vcf.gz test.echtvar0 test.echtvar1
# rm generated-all.vcf generated-subset0.vcf anno.vcf.gz test.echtvar0 test.echtvar1
bash string.sh
Copy link
Owner

Choose a reason for hiding this comment

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

why did you change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I am not ready for re-review yet. I made the change locally and I am using a remote machine to test. I hadn't made my test script yet, but just wanted to see the outputs in action before they are removed. I guess it's a little awkward to work on my own branch under scrutiny (I don't do cross-repo PRs often with external collaborators), but I'll re-request review when I think it's ready.

@migbro
Copy link
Contributor Author

migbro commented Jan 29, 2024

@brentp , so I have added the test, but in the process I was thinking, what if instead of prepending added by echtvar to all the INFO fields when one is provided or scraped (in the future), I instead do something similar like add an echtvar command line, like bcftools and VEP does. It's part of the second commit of https://github.com/brentp/echtvar/pull/34/files/580fb12830f55693dc0dd7d2165e00b9cfacaa91..3c4eca58fca3060d037059ed8083f958b9c26842 and when I tested it out, it came out like:
##echtvar_anno_Command=anno -i None generated-all.vcf anno.vcf.gz -e ["test.echtvar0"]
I can mess around with string a bit ( I am a complete rust novice ). Thoughts? Thanks in advance for your attentiveness!

@brentp
Copy link
Owner

brentp commented Jan 30, 2024

@migbro , nicely done. I am OK with that echtvar command string. You can just join the archives with " -e " to get rid of the square brackets.

Let me know when you're ready for review.

src/lib/echtvar.rs Outdated Show resolved Hide resolved
📝 default to 1 when INFO is A R G
🧪 updated tests
@migbro migbro requested a review from brentp January 30, 2024 21:53
@migbro
Copy link
Contributor Author

migbro commented Jan 30, 2024

@brentp ok, I think I have addressed PR comments and suggestions, since the last look I have:

  • Cleaned up the added command line
  • Convert A R G to 1, for Number, leave all other values alone
  • Updated tests to account for changes

@brentp brentp merged commit d4ab3a8 into brentp:main Jan 31, 2024
1 check passed
@brentp
Copy link
Owner

brentp commented Jan 31, 2024

thank you!
I'll get this out in next release with changes from @dmiller15 in #35

@migbro migbro deleted the v0.2.0-rc branch February 6, 2024 20:23
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