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

V2 CLI: Add external commands #1103

Merged
merged 2 commits into from
Nov 26, 2023
Merged

Conversation

rm-dr
Copy link
Contributor

@rm-dr rm-dr commented Oct 4, 2023

Implemented the feature proposed in #737.
Is it really that easy?

A question:
Should v2_main exit earlier if an external command is given?
We probably don't need to set up colors and customizations if we're running an external command.

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (41168a9) 46.29% compared to head (11df05c) 46.20%.
Report is 44 commits behind head on master.

Files Patch % Lines
src/bin/tectonic/v2cli.rs 3.57% 27 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1103      +/-   ##
==========================================
- Coverage   46.29%   46.20%   -0.09%     
==========================================
  Files         171      171              
  Lines       65111    65099      -12     
==========================================
- Hits        30142    30078      -64     
- Misses      34969    35021      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ColeFrench
Copy link
Contributor

ColeFrench commented Nov 4, 2023

Cool! I think it would be useful to add tests for each target (unix and windows) with a dummy external subcommand. An addition to the documentation describing this feature should also be helpful.

Hm, I wonder if the help subcommand should also list external subcommands. Cargo has a list subcommand that lists both internal and external subcommands. More generally, maybe structopt/clap can integrate with external subcommands...

@pkgw
Copy link
Collaborator

pkgw commented Nov 26, 2023

Thanks! I'm going to close and reopen this to rerun the CI to check that this still builds cleanly against the latest master.

@pkgw pkgw closed this Nov 26, 2023
@pkgw pkgw reopened this Nov 26, 2023
@pkgw
Copy link
Collaborator

pkgw commented Nov 26, 2023

Agreed that it would be good to add test coverage for this, although I admit that it might take a fair amount of work to implement for relatively small benefit. It might be helpful to see if Cargo has a clever, lightweight way for testing this kind of functionality.

One further ask that's a little bit more important to me would be updating the documentation somewhere to mention this capability. I'm not sure if there's a single best place in the docs to describe it, but ideally it should be described somewhere.

@pkgw
Copy link
Collaborator

pkgw commented Nov 26, 2023

The Azure Pipelines CI failures are spurious — a side effect of me merging another PR while this one was running. They should go away with the next fresh CI build.

@pkgw pkgw merged commit da38c79 into tectonic-typesetting:master Nov 26, 2023
26 of 28 checks passed
@rm-dr rm-dr deleted the externalcmd branch November 27, 2023 00:11
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