feat: add analyzeDocuments + SchemaAccessor COMPASS-8799 #216
Conversation
paula-stacho
force-pushed
the
COMPASS-8799
branch
from
c6196fc to
1249f8b
Compare
paula-stacho
changed the title
| options: { | ||
| signal?: AbortSignal | ||
| }): ExtendedJSONSchema { | ||
| // TODO: COMPASS-8702 |
There was a problem hiding this comment.
I'm a bit apprehensive about merging this to main - if we find out we want to do a release of some other fix in the meantime before this TODO is completed then we'd effectively be releasing something that doesn't work.
I don't think we chatted about this in the technical design. How would you feel about having a separate branch that we merge this branch and all of these various accessors to before we merge them into main to avoid that?
Again it's mostly only us using this package, but it can be a good practice to follow. We did a similar thing with the copilot agent in VSCode: mongodb-js/vscode#839
|
|
||
| export type StandardJSONSchema = JSONSchema4; | ||
|
|
||
| export type MongodbJSONSchema = Pick<StandardJSONSchema, 'title' | 'required' | 'description'> & { |
There was a problem hiding this comment.
A total nit question, how do you feel about MongoDBJSONSchema? I suppose it is a lot of uppercase letters in a row though lol. I think we typically write MongoDB with the uppercase DB in our code in Compass, and it looks like the driver does too at least with MongoDBNamespace. Same question with the related functions.
| private signal?: AbortSignal; | ||
|
|
||
| constructor(internalSchema: InternalSchema, signal?: AbortSignal) { | ||
| this.signal = signal; |
There was a problem hiding this comment.
Is there a benefit for us saving the signal here and using it in the accessors verses having the accessors receive the signal as one of their arguments?
I'm thinking it may be more flexible to have the various accessors receive a signal in their function arguments in case the consumer of this wants to say start a schema retrieval, then abort it because the user clicked cancel or something, and then start a different one.
| return source; | ||
| } | ||
|
|
||
| export async function getCompletedSchemaAnalyzer( |
There was a problem hiding this comment.
nit: I like to avoid adding functions to util files when possible as I feel they can sometimes absorb a lot of the code and overtime that can make discovery more difficult. Would it make sense to move these functions to the schema-analyzer.ts file? No changes needed, mostly a thought/suggestion. Also I should mention I like not having much code in index.ts files so I'm happy these are at least not there anymore!
Adding
analyzeDocuments() -> SchemaAccessoras prep for having multiple formats. The interface has been chosen so that:see the tech design for more details.