adding to the stats #1560
adding to the stats #1560
Conversation
if you pass options via , they don't show up in the stats, which can be a problem for debugging
|
I'm pushing this against I've tested this on 110, and it's working fine for me. |
|
Thanks for the update, @dbolser! |
|
Thanks, I'll update the test. How to decide which branch to use when
creating a PR?
…On Wed, Nov 22, 2023, 6:06 PM Diana Lemos ***@***.***> wrote:
Thanks for the update, @dbolser <https://github.com/dbolser>!
The reason why the tests are failing is this one:
https://github.com/Ensembl/ensembl-vep/blob/release/112/t/Stats.t#L409-L423
In the test the expected output doesn't include the new 'Full command line
options'. This test needs to be updated to reflect the new behaviour.
Can you also please change the base to postreleasefix/112?
—
Reply to this email directly, view it on GitHub
<#1560 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA7NE7IXV4IZRB4XZ35P4LYFY5KJAVCNFSM6AAAAAA7TASSSCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRTGI2DINBZGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
The branch is the latest postreleasefix which right now is postreleasefix/112. |
|
Right, but is it the right branch for me to PR against?
Last time I did a PR against the default branch, then was asked to move it
to a prereleasebugfix/ (IIRC)... Is there a quick tutorial saying which
branch to PR against?
Many thanks,
…On Thu, 23 Nov 2023 at 10:16, Diana Lemos ***@***.***> wrote:
The branch is the latest postreleasefix which right now is
postreleasefix/112.
—
Reply to this email directly, view it on GitHub
<#1560 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA7NE536M7GZM64RW2CWHLYF4O6NAVCNFSM6AAAAAA7TASSSCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRUGEZDKNZSGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| ['Command line options', '<pre>'.join(" ", @opts).'</pre>'], | ||
| ['Full command line options', '<pre>'.$self->config->{_params}->{full_command}.'</pre>'], |
There was a problem hiding this comment.
Hey @dbolser,
Thanks for your PR.
It depends on our internal release schedule, but usually any PR should be merged into the latest postreleasefix branch.
Regarding your changes, I think it would be better to simply replace the functionality of Command line options, so we return the same string as in the VCF output. What do you think?
| ['Command line options', '<pre>'.join(" ", @opts).'</pre>'], | |
| ['Full command line options', '<pre>'.$self->config->{_params}->{full_command}.'</pre>'], | |
| ['Command line options', '<pre>'.$self->config->{_params}->{full_command}.'</pre>'], |
Let me know if you have any other questions. Thanks!
Best regards,
Nuno
There was a problem hiding this comment.
Hi @dbolser, are you fine with me making the change that I suggested? Let me know, thanks!
if you pass options via
--config my_config.ini, they don't show up in the stats, which can be a problem for debugging