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.

Sign up for GitHub

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

Jump to bottom

Refactor Bindgen to simpler form #18

Merged
merged 8 commits into from
Nov 1, 2024
Merged

Refactor Bindgen to simpler form #18

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cfc35d7
Refactor Bindgen to simpler form
bhelx Oct 25, 2024
000bc0d
fix validation, restrict map value types
bhelx Oct 29, 2024
40082b9
restrict arrays as well
bhelx Oct 29, 2024
0e08154
remove restrictions on nested types
bhelx Oct 29, 2024
63f8453
more tests
bhelx Oct 29, 2024
c065934
Remove required from type, prefer required on property
bhelx Nov 1, 2024
13879c4
add warnings property as part of response
bhelx Nov 1, 2024
9a1e0ea
bump to rc12
bhelx Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 52 additions & 25 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@
"devDependencies": {
"@extism/js-pdk": "^1.0.1",
"@types/jest": "^29.5.12",
"@types/js-yaml": "^4.0.9",
"@types/node": "^22.8.1",
"const": "^1.0.0",
"esbuild": "^0.17.0",
"esbuild-plugin-d.ts": "^1.2.3",
"jest": "^29.0.0",
"js-yaml": "^4.1.0",
"ts-jest": "^29.0.0",
"typescript": "^5.0.0"
"typescript": "^5.6.3"
},
"files": [
"dist"
Expand Down
10 changes: 5 additions & 5 deletions src/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export class ValidationError extends Error {
constructor(public message: string, public path: string) {
super(message);
Object.setPrototypeOf(this, ValidationError.prototype);
}
export class ValidationError {
constructor(public message: string, public path: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to throw exceptions and stop the process, we want to collect all validation errors giving the user the chance to address them all in one.

Copy link
Contributor

Choose a reason for hiding this comment

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

love this!

this.message = message
this.path = path
}
}
53 changes: 50 additions & 3 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import {
XtpSchema,
} from "./normalizer";
import { CodeSample } from "./parser";
import { XtpNormalizedType } from "./types";
export * from "./normalizer";
export * from "./types";
export { ValidationError } from "./common";

export function parse(schema: string) {
Expand Down Expand Up @@ -104,9 +106,43 @@ function isPrimitive(p: Property | Parameter): boolean {
return !!p.$ref.enum || !p.$ref.properties;
}

function isDateTime(p: Property | Parameter | null): boolean {
if (!p) return false;
return p.type === "string" && p.format === "date-time";
export type XtpTyped = { xtpType: XtpNormalizedType };
Copy link
Contributor Author

@bhelx bhelx Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is an external convenience type for any node in the doc that should have an xtp type. think schema, property, parameter, items, additionalProperties, etc.


function isDateTime(p: XtpTyped): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are helpers to quickly check the type of a node without drilling into the xtpType.kind. Will work on any XtpTyped object.

return p?.xtpType?.kind === 'date-time'
}
function isBuffer(p: XtpTyped): boolean {
return p?.xtpType?.kind === "buffer"
}
function isObject(p: XtpTyped): boolean {
return p?.xtpType?.kind === "object"
}
function isArray(p: XtpTyped): boolean {
return p?.xtpType?.kind === "array"
}
function isEnum(p: XtpTyped): boolean {
return p?.xtpType?.kind === "enum"
}
function isString(p: XtpTyped): boolean {
return p?.xtpType?.kind === "string"
}
function isInt32(p: XtpTyped): boolean {
return p?.xtpType?.kind === "int32"
}
function isInt64(p: XtpTyped): boolean {
return p?.xtpType?.kind === "int64"
}
function isFloat(p: XtpTyped): boolean {
return p?.xtpType?.kind === "float"
}
function isDouble(p: XtpTyped): boolean {
return p?.xtpType?.kind === "double"
}
function isBoolean(p: XtpTyped): boolean {
return p?.xtpType?.kind === "boolean"
}
function isMap(p: XtpTyped): boolean {
return p?.xtpType?.kind === "map"
Copy link

Choose a reason for hiding this comment

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

If I had something like:

type: object
properties:
  foo:
    type: string
    nullable: false
  bar:
    type: number
additionalProperties:
  type: 'string'

Would that be a map or an object? (Does this change for patternProperties?)

(Edit: I get into semi-structured objects a little later in the review at MapType!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be caught in the validation step as these two concepts should not be mixable

Copy link
Contributor

@mhmd-azeez mhmd-azeez Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, currently we don't support that. additionalProperties and properties are mutually exclusive

}

function capitalize(s: string) {
Expand Down Expand Up @@ -135,6 +171,17 @@ export const helpers = {
codeSamples,
isDateTime,
isPrimitive,
isBuffer,
isObject,
isEnum,
isArray,
isString,
isInt32,
isInt64,
isFloat,
isDouble,
isMap,
isBoolean,
isJsonEncoded,
isUtf8Encoded,
capitalize,
Expand Down
Loading
Loading