-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[OV JS] Add TS definition for AnyMap #26711
Conversation
src/bindings/js/node/lib/addon.ts
Outdated
@@ -21,6 +21,12 @@ type elementTypeString = | |||
| 'f32' | |||
| 'string'; | |||
|
|||
type Any = string | number | boolean; |
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.
I propose to choose different name, because Any
crosses with any
in TypeScript definition.
It can be longer, but cleaner, for example: AllowedPropertyType
, ConfigPropertyType
, etc
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.
I updated the PR description and I think the best name will be OVAny
src/bindings/js/node/lib/addon.ts
Outdated
type AnyMap = { | ||
[propertyName: string]: Any; | ||
}; |
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.
type AnyMap = { | |
[propertyName: string]: Any; | |
}; | |
type AnyMap = Record<string, *AnyNewName*>; |
https://www.typescriptlang.org/docs/handbook/utility-types.html#recordkeys-type
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.
Also, propose to replace AnyMap
name
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.
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.
I removed definition of AnyMap
. Agree that built-in Record
type is a simpler way to represent a map of string keys to OVAny values and provides better hints.
### Details: - Add definition of `OVAny` to `addon.ts` to encapsulate the concept of [ov::Any](https://docs.openvino.ai/2024/api/c_cpp_api/classov_1_1_any.html) from C++ API in a way that's clear and can be easily updated if the underlying C++ type changes. Improved readability and maintainability. -- 'OVAny' corresponds to Python API [OVAny](https://docs.openvino.ai/2024/api/ie_python_api/_autosummary/openvino.OVAny.html) - In future updates to Node.js API, the OVAny type is expected to be enhanced with additional functionality. - Missed linter changes to `core.test.js` due to upstream merges ### Tickets: - [CVS-149319](https://jira.devtools.intel.com/browse/CVS-149319)
Details:
OVAny
toaddon.ts
to encapsulate the concept of ov::Any from C++ API in a way that's clear and can be easily updated if the underlying C++ type changes. Improved readability and maintainability.-- 'OVAny' corresponds to Python API OVAny
core.test.js
due to upstream mergesTickets: