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

⚙️ Use simple validators in TOC #1172

Merged
merged 7 commits into from
May 7, 2024

Conversation

agoose77
Copy link
Contributor

As we want to integrate TOC validation with our config system, and raise informative errors, this PR drops schema generation in favour of explicit imperative validation (until we get around to moving to a schema/ORM based approach).

@agoose77 agoose77 requested a review from rowanc1 April 30, 2024 17:48
@agoose77 agoose77 marked this pull request as ready for review April 30, 2024 17:48
Copy link
Member

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

Verbosity 🚀. @fwkoch I think your review here would be helpful.

"test:watch": "vitest watch",
"build:esm": "tsc",
"build:schema": "npx ts-json-schema-generator --path src/types.ts --type TOC -o ./src/schema.json",
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we could keep this around, even if we aren't using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure but we don't publish it yet. I'm thinking let's add it later?

packages/myst-toc/src/toc.ts Outdated Show resolved Hide resolved
packages/myst-toc/tests/examples.spec.ts Show resolved Hide resolved
Copy link
Collaborator

@fwkoch fwkoch left a comment

Choose a reason for hiding this comment

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

Looks nice - I suggested some changes to tighten up the types so we don't accidentally let errors sneak through. Let me know if you have questions...

packages/myst-toc/src/toc.ts Outdated Show resolved Hide resolved
outputEntry.file = validateString(outputEntry.file, incrementOptions('file', opts));

outputEntry = validateCommonEntry(outputEntry, opts);
if (!outputEntry) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't think this is possible with how validateCommonEntry is defined (see the corresponding type updates I made).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer, indeed!

packages/myst-toc/src/toc.ts Outdated Show resolved Hide resolved
packages/myst-toc/src/toc.ts Outdated Show resolved Hide resolved
packages/myst-toc/src/toc.ts Outdated Show resolved Hide resolved
@agoose77 agoose77 force-pushed the agoose77/refactor-use-simple-validators branch 2 times, most recently from f20ce45 to 3d3e914 Compare May 3, 2024 09:57
@agoose77
Copy link
Contributor Author

agoose77 commented May 3, 2024

@fwkoch I've made some edits following the suggestions that you and @rowanc1 made! Would you mind another glance and then merging if happy?

@agoose77 agoose77 requested a review from fwkoch May 7, 2024 07:55
@agoose77 agoose77 force-pushed the agoose77/refactor-use-simple-validators branch from 2018149 to ad50fcc Compare May 7, 2024 08:00
@fwkoch fwkoch merged commit 8463044 into main May 7, 2024
5 checks passed
@fwkoch fwkoch deleted the agoose77/refactor-use-simple-validators branch May 7, 2024 16:07
@agoose77 agoose77 self-assigned this May 7, 2024
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