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: add analyzeDocuments + SchemaAccessor COMPASS-8799 #216

Merged

Conversation

paula-stacho
Copy link

@paula-stacho paula-stacho commented Jan 16, 2025

Adding analyzeDocuments() -> SchemaAccessor as prep for having multiple formats. The interface has been chosen so that:

  • the conversion to different formats happens only when requested, and only once
  • if we switch to analysis which uses $jsonSchema as the base format, the interface can stay the same

see the tech design for more details.

@paula-stacho paula-stacho marked this pull request as ready for review January 16, 2025 12:16
@paula-stacho paula-stacho changed the title feat: add SchemaAccessor COMPASS-8799 feat: add analyzeDocuments + SchemaAccessor COMPASS-8799 Jan 16, 2025
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Looking good, left a couple comments/suggestions

src/schema-convertors.ts Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/schema-accessor.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
.depcheckrc Show resolved Hide resolved
@paula-stacho paula-stacho changed the base branch from main to COMPASS-6862-schema-export-multiple-formats January 20, 2025 11:50
Comment on lines 47 to 48
if (this.extendedJSONSchema) return this.extendedJSONSchema;
return this.extendedJSONSchema = await convertors.internalSchemaToExtended(this.internalSchema, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion, but these could all be e.g.

Suggested change
if (this.extendedJSONSchema) return this.extendedJSONSchema;
return this.extendedJSONSchema = await convertors.internalSchemaToExtended(this.internalSchema, options);
return this.extendedJSONSchema ??= await convertors.internalSchemaToExtended(this.internalSchema, options);

right?

Copy link
Author

Choose a reason for hiding this comment

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

right, I keep forgetting this exists. thank you!

src/schema-analyzer.ts Outdated Show resolved Hide resolved
@paula-stacho paula-stacho merged commit 591fa39 into COMPASS-6862-schema-export-multiple-formats Jan 22, 2025
18 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8799 branch January 22, 2025 08:49
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