-
Notifications
You must be signed in to change notification settings - Fork 12
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(service-spec-importers): expose importers via DatabaseBuilder #631
Conversation
96e17ac
to
1dd2845
Compare
1dd2845
to
df5fe20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditionally approved
.importScrutinies() | ||
.importAugmentations() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these built into the importers package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately yes. Should we change that? WDYT?
export type SourceImporter = (db: SpecDatabase, report: ProblemReport) => Promise<void>; | ||
|
||
export class DatabaseBuilder { | ||
private readonly sourceImporters = new Array<SourceImporter>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this lazy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly to improve UX, this enables a fluent interface. Otherwise it's a bunch of statements that need to be awaited.
I don't have strong feelings about this. It made sense to me to expose the importers as methods on a class.
import { SpecDatabase, emptyDatabase } from '@aws-cdk/service-spec-types'; | ||
import { DatabaseBuilder, DatabaseBuilderOptions } from './db-builder'; | ||
|
||
const SOURCES = path.join(__dirname, '../../../../sources'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me think this file needs to live somewhere else. Can it be an unpackaged build tool for aws-service-spec
?
Otherwise, make this directory a parameter I think and call it with the right relative directory from the aws-service-spec
build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes planning to do exactly this in the next PR!
Preparing the
service-spec-importers
package for a public release, pt2.This change replaces existing exports of everything with explicit exports for:
DatabaseBuilder
a new class that helps importing data into a databaseDbDiff
a class that helps diff'ing to databasesProblemReport
a class to collect & report problems encountered during importWhile this change is already enough to import additional specs on top of an existing database, we will need one further change to decouple the package from the source files in this repo.