search

qiime2 / q2cli

Command line interface for QIIME 2
BSD 3-Clause "New" or "Revised" License
19 stars 24 forks source link

refactor: usage renderer logic is now fully priv #252

Closed thermokarst closed 3 years ago

thermokarst commented 3 years ago

this work came up in a discussion with @andrewsanchez - having a render method on all of the usage drivers would help simplify his life when building sphinx-ext-qiime2. I plan to follow up to make that render method part of the base Usage class API.

Also, while working on this I had some ideas for refactoring the CLI-specific usage cache, more on that later.

thermokarst commented 3 years ago

I think I'm going to take a stab at refactoring CLIUsage along the lines of the DiagnosticUsage refactor in https://github.com/qiime2/qiime2/pull/561

thermokarst commented 3 years ago

I think I'm going to take a stab at refactoring

Just kidding.

andrewsanchez commented 3 years ago

Everything looks good to me, but can you push a hack commit then revert it to get the workflow to run? I don't think I can trigger (and I can't push to thermokarst-forks) it manually since it hasn't run at all due to the previous draft status. Tests and everything pass locally, but I'd still like to see the CI pass before merging 🤓 .

I like the class method and the idea of instantiating a stateful driver from some serializable input 🧠 .

thermokarst commented 3 years ago

Everything looks good to me,

Thanks!

but can you push a hack commit then revert it to get the workflow to run

I can, but you'll be waiting a looonnnggg time - remember, our travis ci acct is disabled due to overuse! ☠️

Screen Shot 2021-02-15 at 7 07 40 PM

If travis ci was working for us, no need for a new commit, you could just go into the travis UI and re-run the job.

and I can't push to thermokarst-forks

Can you try again (but remember, the ci is disabled) - I should have the "allow commits from maintainers" feature on (I never turn that off when I open PRs), but your acct had read-only permissions on this repo, which conflicted with your org-wide permissions. I removed those read-only perms, so in theory you should be set. The one "gotcha" though is that thermokarst-forks is an org, not an individual acct, so the "allow commits from maintainers" might not apply here.

I'd still like to see the CI pass before merging

Without travis, your best bet is to run locally!

I like the class method and the idea of instantiating a stateful driver from some serializable input

Thanks! 💝