-
Notifications
You must be signed in to change notification settings - Fork 57
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
Atdts: supporting <ocaml from ...> annotation #429
base: master
Are you sure you want to change the base?
Conversation
@ygrek The feature is to be able to pass an OCaml module that is converted to the ts equivalent type or just have modularity to import types from another .atd file? |
@@ -649,7 +649,7 @@ let get_ts_default (e : type_expr) (an : annot) : string option = | |||
| None -> get_default_default e | |||
|
|||
(* piggy-back on ocaml annotations TODO check ts ones first *) | |||
let get_annot an field = Atd.Annot.get_opt_field ~parse:(fun s -> Some s) ~sections:["ocaml"] ~field an | |||
let get_annot an field = Atd.Annot.get_opt_field ~parse:(fun s -> Some s) ~sections:["ts"] ~field an |
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 comment is now wrong
but also it is convenient to implement what this comment says - if there is no ts annot for from - use ocaml one. The reason this makes sense is because abstract
without from
gets generated as any
in ts which is not very useful (I would consider actually turning it into error because from what i have seen 100% it is programmer mistake not intention)
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 disagree about reading <ocaml ...>
annotations with atdts. TypeScript and OCaml should not know about each other. When there are similarities that are more or less universal, we should handle them in a language-neutral fashion (= no reference to other target languages).
See also #265 for a dedicated import
syntax that turned out to be too complicated to implement in atdgen but is reasonable for the other target languages. The long-term solution I propose is to implement atdml (but unfortunately it would take me a solid 3 weeks to make something usable). See the comment I just left on my original PR attempt: #297 (comment)
|
||
Position: left-hand side of a type definition, after the type name | ||
|
||
Values: .ts file with exported types. This can be also seen as the |
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 believe this is documented in the general atd language documentation? (maybe mentioning in ts doc is useful but still should link/refer to the main doc)
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.
The annotation is <ts ...>
, so it is specific to the TypeScript target and should be documented here.
@@ -3,6 +3,7 @@ Unreleased | |||
|
|||
* atdgen: Add option `-j-gen-modules` to generate JSON generic submodules (#420) | |||
* atd-parser: improve (syntax) error messages (#426) | |||
* atdts: support <ocaml from...> annotation |
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 should be <ocaml ts=...>
. I would add a slightly longer note explaining what the feature is about.
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.
<ts from=..>
?
PR checklist
CHANGES.md
is up-to-date