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

fix(Settings): use schema #3375

Merged
merged 6 commits into from
Oct 30, 2021
Merged

fix(Settings): use schema #3375

merged 6 commits into from
Oct 30, 2021

Conversation

ylemkimon
Copy link
Member

@ylemkimon ylemkimon commented Oct 30, 2021

What is the previous behavior before this PR?

The options of Settings were repeated in cli.js, type SettingsOptions, class Settings, and Settings::constructor(). This caused some options missing in the CLI, #2237, #2428, and minRuleThickness.

What is the new behavior after this PR?

DRY options in cli.js, Settings::constructor(), and type SettingsOptions. Use SETTINGS_SCHEMA and $Shape<Settings>, respectively.

This adds -f, --format <type> and --min-rule-thickness <size> to the CLI. Fixes #2237 and closes #2241. It also exports SETTINGS_SCHEMA, so developers and users could use.

Currently, Flow checks if a non-existent property on Settings exists on the schema.

In the future, we could

  • validate options against the schema, throwing on unknown values or type mismatches (this would be a breaking change)
  • automatically generate Options docs from the schema
  • check all properties on Settings exist on the schema
  • check the type of a property on Settings matches the schema (I'd like to do these two after Consider using TypeScript instead of Flow #2110)

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2021

Codecov Report

Merging #3375 (c2fe15a) into main (15ee9b4) will decrease coverage by 0.08%.
The diff coverage is 77.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3375       /-   ##
==========================================
- Coverage   93.54%   93.46%   -0.09%     
==========================================
  Files          88       88              
  Lines        6558     6575       17     
  Branches     1513     1525       12     
==========================================
  Hits         6135     6145       10     
- Misses        393      399        6     
- Partials       30       31        1     
Impacted Files Coverage Δ
src/Settings.js 73.52% <77.41%> (-4.91%) ⬇️

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 23b3950...c2fe15a. Read the comment docs.

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.

Wow, this is an impressive rewrite; it shows the extensive complexity of our options system.

I did a bunch of testing of cli (should we have automated tests for this??), and found one bug/change in the behavior of -f. Otherwise looks good.

src/Settings.js Outdated
output: {
type: {enum: ["htmlAndMathml", "html", "mathml"]},
description: "Determines the markup language of the output.",
cli: "-f, --format <type>",
Copy link
Member

Choose a reason for hiding this comment

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

-f conflicts with --macro-file. I guess we should remove it for backward compatibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's go with -F.

@ylemkimon
Copy link
Member Author

should we have automated tests for this?

Tracked in #1503.

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.

Thanks for all your recent PRs!

@ylemkimon ylemkimon merged commit b58a432 into main Oct 30, 2021
@ylemkimon ylemkimon deleted the settings-schema branch October 30, 2021 19:41
@ylemkimon
Copy link
Member Author

Thank you for all the reviews!

KaTeX-bot added a commit that referenced this pull request Oct 30, 2021
## [0.14.1](v0.14.0...v0.14.1) (2021-10-30)

### Bug Fixes

* **Settings:** use schema ([#3375](#3375)) ([b58a432](b58a432))
@KaTeX-bot
Copy link
Member

🎉 This PR is included in version 0.14.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Allow to set output format in CLI
4 participants