Skip to content

Add type check - #80

Merged
daogrady merged 14 commits into
mainfrom
feat/types
Aug 22, 2025
Merged

Add type check#80
daogrady merged 14 commits into
mainfrom
feat/types

Conversation

@daogrady

@daogrady daogrady commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

Introducing strict types and a strict checking mechanism is a first step towards facilitating a better understanding, cleaning up the current file structure, and enabling subsequent refactoring efforts.

This PR's scope is to introduce a consistent checking mechanism, make types slightly stricter where easy wins can be achieved, but leave the current structure untouched as much as possible.
Making the types as strict as possible will be done in a follow-up PR.

@daogrady
daogrady requested review from swaldmann and tim-sh August 21, 2025 13:12
@daogrady
daogrady marked this pull request as ready for review August 21, 2025 13:13
Comment thread lib/compile/types.d.ts
type AllOf = { allOf: Schema[] } & Meta
type MultiSchema = AnyOf | AllOf

export type Schema = (SingleSchema | MultiSchema)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Beautiful! 🤩

Comment thread lib/compile/csdl2openapi.js Outdated
Comment thread lib/compile/csdl2openapi.js Outdated
Comment thread lib/compile/csdl2openapi.js Outdated
Comment thread lib/compile/csdl2openapi.js Outdated
@@ -1170,7 +1174,7 @@ see [Expand](http://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part1-prot
.map(entryToProperty({ path: prefix, typeRefChain: [] }));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be worthwhile to type entryToProperty return value? This would make the change in l. 1177 obsolete.

* Add parameter for query option $skip
* @param {Array} parameters Array of parameters to augment
* @param {string} target Target container child of path
* @param {Record<string, boolean>} target Target container child of path FIXME: this seems to be an incorrect use of TargetRestrictions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

target not explicitly typed including null, as is the case in l. 1252…

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe target should have the same type as in all other instances where it is declared as parameter. But the usage within the function implies a different type. I highly suspect that this is actually a bug in the implementation (see the FIXME behind the type), but I did not want to introduce behavioural changes in this PR.

Comment thread lib/compile/csdl2openapi.js Outdated
Comment thread lib/compile/csdl2openapi.js Outdated
checkForExtentionEnums(extensionObj, extensionEnums);

let extenstionSchema = {
let extenstionSchema = {

@swaldmann swaldmann Aug 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a regression in this PR, but rename to extensionSchema + the function above (checkForExtentionEnums)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Woops, good catch, renamed to eggstensioné.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Éééé, bièn choisi cosí! 🥖 🇮🇹

Comment thread lib/compile/csdl2openapi.js
@daogrady
daogrady enabled auto-merge (squash) August 22, 2025 07:26
@daogrady
daogrady merged commit c2ff8c1 into main Aug 22, 2025
5 checks passed
@daogrady
daogrady deleted the feat/types branch August 22, 2025 07:26
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