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

feat(cli): --trust flag for trusting cli input #3339

Merged
merged 5 commits into from Oct 26, 2021

Conversation

ngzhian
Copy link
Contributor

@ngzhian ngzhian commented Oct 22, 2021

The trust option has much finer grained control, which is hard to
specify on the command line, so expose just a single boolean flag, which
changes trust from false (default) to true.

The trust option has much finer grained control, which is hard to
specify on the command line, so expose just a single boolean flag, which
changes trust from false (default) to true.
@edemaine
Copy link
Member

Is this a work-in-progress? The new option doesn't seem to be getting used/checked anywhere.

I also wonder whether --trust-all (or even --trust) would be a better spelling for the option. There's only one input in an execution of the CLI, so it doesn't seem to make sense to call out inputs.

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2021

Codecov Report

Merging #3339 (5269823) into master (b1e24f2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3339   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files          88       88           
  Lines        6552     6552           
  Branches     1509     1509           
=======================================
  Hits         6129     6129           
  Misses        393      393           
  Partials       30       30           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1e24f2...5269823. Read the comment docs.

@ngzhian
Copy link
Contributor Author

ngzhian commented Oct 25, 2021

Is this a work-in-progress? The new option doesn't seem to be getting used/checked anywhere.

Is there a particular place I should add the test to? I'm not sure where such cli tests should go.

I also wonder whether --trust-all (or even --trust) would be a better spelling for the option. There's only one input in an execution of the CLI, so it doesn't seem to make sense to call out inputs.

--trust-all looks good, I can change it

@edemaine
Copy link
Member

I wasn't talking about tests. I mean that you never actually look at options.trustAllInputs, so your code doesn't "do" anything, other than accept a new argument. The trust option remains unaffected.

@ngzhian
Copy link
Contributor Author

ngzhian commented Oct 25, 2021

I wasn't talking about tests. I mean that you never actually look at options.trustAllInputs, so your code doesn't "do" anything, other than accept a new argument. The trust option remains unaffected.

Right, thanks! I will rename it to trust, then program.parse(process.argv) should take care of it, since SettingOptions.trust takes a boolean as well.

@edemaine
Copy link
Member

Cool, this does seem consistent with how --strict works. By that analogy, perhaps we should add -T too?

@ngzhian
Copy link
Contributor Author

ngzhian commented Oct 25, 2021

Done!

Copy link
Member

@edemaine edemaine left a comment

Choose a reason for hiding this comment

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

I just tested that this works for me (with a \url input). Awesome!

I forgot but confirmed that the documentation is generated automatically from cli.js itself. See https://deploy-preview-3339--katex.netlify.app/docs/cli.html

I just revised the documentation; assuming you approve, I'll merge.

Update: I found PR #2839 which is identical, so I'll add @summersz as a coauthor when merging.

@edemaine edemaine changed the title feat: allow trusting all inputs via cli flag feat(cli): --trust flag for trusting cli input Oct 25, 2021
@edemaine edemaine linked an issue Oct 25, 2021 that may be closed by this pull request
@ngzhian
Copy link
Contributor Author

ngzhian commented Oct 26, 2021

Thanks for improving the documentation and finding the older issue, looks great, please merge whenever convenient, thanks!

@edemaine edemaine merged commit 503f7d7 into KaTeX:master Oct 26, 2021
KaTeX-bot added a commit that referenced this pull request Oct 26, 2021
## [0.13.19](v0.13.18...v0.13.19) (2021-10-26)

### Features

* **cli:** --trust flag for trusting cli input ([#3339](#3339)) ([503f7d7](503f7d7)), closes [#2428](#2428)
@KaTeX-bot
Copy link
Member

🎉 This PR is included in version 0.13.19 🎉

The release is available on:

Your semantic-release bot 📦🚀

@edemaine
Copy link
Member

@ngzhian Thanks for the PR!

@ngzhian ngzhian deleted the trust-option-on-cli branch October 26, 2021 22:47
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.

Add --trust support to cli
4 participants