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

🔧 Tighten up JSON input #1405

Merged
merged 8 commits into from
Jul 30, 2024
Merged

🔧 Tighten up JSON input #1405

merged 8 commits into from
Jul 30, 2024

Conversation

agoose77
Copy link
Contributor

This PR adds some validation for JSON-file input, and applies our frontmatter transforms.

opts,
);
// Although this is required, we'll set it to an empty article by default
const mdast: GenericParent = value?.mdast ?? { type: 'root', children: [] };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a reasonable thing to do? Declare this mdast property required, but add a default? I did this so that we don't need to throw an error in this function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although maybe throwing an error (or at least returning undefined) would be better?

Both of these results are handled gracefully in the loadFile function where this is called, and then we wouldn't end up persisting and processing this likely erroneous empty page?

@agoose77 agoose77 requested a review from rowanc1 July 23, 2024 17:01
@agoose77
Copy link
Contributor Author

@rowanc1 this follows from our meeting, and fixes the frontmatter.

file: vfile.path,
messages: {},
errorLogFn: (message: string) => {
fileError(vfile, message, { ruleId: opts?.ruleId ?? RuleId.mystJsonValid });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of the rule ID follows our existing convention.

@agoose77 agoose77 requested review from fwkoch and removed request for rowanc1 July 25, 2024 11:31
@agoose77
Copy link
Contributor Author

@fwkoch friendly ping! :)

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.

Sorry to be slow on this! A few comments, hopefully they make sense. In particular, I think just throwing an error during validation would be fine (and probably better).

opts,
);
// Although this is required, we'll set it to an empty article by default
const mdast: GenericParent = value?.mdast ?? { type: 'root', children: [] };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although maybe throwing an error (or at least returning undefined) would be better?

Both of these results are handled gracefully in the loadFile function where this is called, and then we wouldn't end up persisting and processing this likely erroneous empty page?

packages/myst-cli/src/process/file.ts Outdated Show resolved Hide resolved
packages/myst-cli/src/process/file.ts Outdated Show resolved Hide resolved
packages/myst-cli/src/process/file.ts Show resolved Hide resolved
packages/myst-cli/src/process/file.ts Outdated Show resolved Hide resolved
Copy link

changeset-bot bot commented Jul 30, 2024

🦋 Changeset detected

Latest commit: bdad6fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
myst-common Patch
myst-cli Patch
myst-config Patch
myst-frontmatter Patch
myst-spec-ext Patch
mystmd Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@agoose77
Copy link
Contributor Author

Fab! Thanks @fwkoch, will merge this in!

@agoose77 agoose77 merged commit 3fd53be into main Jul 30, 2024
7 checks passed
@agoose77 agoose77 deleted the agoose77/feat-validate-json-file branch July 30, 2024 12:22
@agoose77 agoose77 self-assigned this Jul 31, 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.

2 participants