Skip to content

Atdts: supporting <ts from ...> annotation#429

Merged
koonwen merged 14 commits intomasterfrom
atdts/support-ocaml-from
May 16, 2025
Merged

Atdts: supporting <ts from ...> annotation#429
koonwen merged 14 commits intomasterfrom
atdts/support-ocaml-from

Conversation

@koonwen
Copy link
Contributor

@koonwen koonwen commented Apr 4, 2025

PR checklist

  • New code has tests to catch future regressions
  • Documentation is up-to-date
  • CHANGES.md is up-to-date

@koonwen
Copy link
Contributor Author

koonwen commented Apr 4, 2025

@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?


(* 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
Copy link
Contributor

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)

Copy link
Collaborator

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
Copy link
Contributor

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)

Copy link
Collaborator

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.

CHANGES.md Outdated

* atdgen: Add option `-j-gen-modules` to generate JSON generic submodules (#420)
* atd-parser: improve (syntax) error messages (#426)
* atdts: support <ocaml from...> annotation
Copy link
Collaborator

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

<ts from=..> ?

@koonwen
Copy link
Contributor Author

koonwen commented Apr 19, 2025

Thanks for the comments @mjambon @ygrek, I've cleaned up the code by removing the references to ocaml. Also rewired the build system so that we don't need a cleanup-for-dune to remove filesystem state have for dune work as expected.

One thing that was surprising was that even though "ocaml" was not specified as a annot_schema in

let annot_schema_ts : Atd.Annot.schema_section =
{
section = "ts";
fields = [
Type_expr, "repr";
Field, "default";
Type_def, "from";
Type_def, "t";
]
}
let annot_schema : Atd.Annot.schema =
annot_schema_ts :: Atd.Json.annot_schema_json
the initial validation pass when the .atd file is loaded doesn't raise an error.

It looks like the annotation validation pass only tries to check if the section was defined in annot_schema and allows the rest through which seems wrong? I.e. if annot_schema contains {section="ocaml"; fields=[]}, it would raise an error since no annotations are allowed for this section. But now if I don't provide the ocaml section, and try to annotate <ocaml from=...>, it passes.

Copy link
Contributor

@smondet smondet left a comment

Choose a reason for hiding this comment

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

LGTM modulo doc typos

doc/atdts.rst Outdated
name of the original ATD file, without the ``.atd`` extension and
capitalized like an OCaml module name.

Semantics: specifies the base name of the OCaml modules where the type
Copy link
Contributor

Choose a reason for hiding this comment

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

s/OCaml/ATD/ ?

doc/atdts.rst Outdated

.. code:: ocaml

type point_xy <ocaml from="Part1" t="point"> = abstract
Copy link
Contributor

Choose a reason for hiding this comment

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

<ts

@koonwen
Copy link
Contributor Author

koonwen commented May 16, 2025

Updated doc types, thanks @smondet. Merging

@koonwen koonwen changed the title Atdts: supporting <ocaml from ...> annotation Atdts: supporting <ts from ...> annotation May 16, 2025
@koonwen koonwen merged commit 759b482 into master May 16, 2025
2 of 3 checks passed
@Khady Khady deleted the atdts/support-ocaml-from branch May 27, 2025 05:47
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.

5 participants