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.

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

Fix code snippet compile errors #211

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

feihong
Copy link
Collaborator

@feihong feihong commented Dec 6, 2024

Also fix:

  • Formatting of reason snippets (because 3.14 formats records a bit differently)
  • Properly convert snippets that contain type signatures

process_md.ml has been changed so that when it detects a snippet that begins with "val ", it assumes that it contains type signatures and wraps it with module type _FAKE_ = sig and end. After it converts to reason code, the extra lines are removed.

@feihong feihong marked this pull request as ready for review December 9, 2024 01:50
@feihong feihong requested review from jchavarri and davesnx December 9, 2024 01:50
Copy link
Member

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Very nice. Thanks so much!

let to_re str =
let is_type_signature, str =
(* assume that any snippet starting with "val" is a partial type signature *)
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

Comment on lines +82 to +83
Error: This expression has type int -> int -> int
but an expression was expected of type ('a -> 'b -> 'c [@u])
Copy link
Member

Choose a reason for hiding this comment

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

Is this expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's expected. This example is supposed to not compile because map expects an uncurried function as its third argument, but add is curried.

It's sometimes hard to figure out which snippet is the one that's supposed to break because they aren't extracted in the order that they appear in the markdown file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added comments in the prelude of snippets that aren't expected to compile in f79420b

```ocaml
val actionToJs : action -> int

val actionFromJs : int -> action option
```
```reasonml
external actionToJs: action => int = ;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refmt translates val actionToJs : action -> int to external actionToJs: action => int = ;. Is it a bug?

Copy link
Member

Choose a reason for hiding this comment

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

idk but certainly seems syntactically broken cc @davesnx @anmonteiro

@@ -335,12 +349,12 @@ example, the OCaml signature would look like this after preprocessing:
```ocaml
type person

val person : name:string -> ?age:int -> unit -> person
external person : name:string -> ?age:int -> unit -> person = "person"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ppx actually doesn't put "person" at the end, but I did so to silence the warning from the compiler

@@ -217,9 +231,9 @@ external route :
_type:string ->
path:string ->
action:(string list -> unit) ->
?options:< .. > ->
?options:'a ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried multiple times to get this snippet to work with Js.t, but kept getting some variation of this error:

Error The type of this expression, (< .. > as '_weak1) Js.t,
contains the non-generalizable type variable(s): '_weak1.
(see manual section 6.1.2)

See this playground snippet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rewrote this example based on the feedback I got in our conversation: f13706e

@@ -1014,15 +1045,22 @@ If the values are strings, we can use the `mel.string` attribute:

```ocaml
external read_file_sync :
name:string -> ([ `utf8 | `ascii ][@mel.string]) -> string = "readFileSync"
name:string -> ([ `utf8 | `ascii ]) -> string = "readFileSync"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keeping the [@mel.string] generates a warning

File "input.ml", line 4, characters 0-33:
4 | val actionToJs : action -> string
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Value declarations are only allowed in signatures
Copy link
Collaborator Author

@feihong feihong Dec 9, 2024

Choose a reason for hiding this comment

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

These errors remain because I throw away the beginning (module type _FAKE_ sig ) and end ( end) when transforming type signatures snippets in process_md.ml. It's probably fixable, but I'm not sure if it's a big deal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't fix it but added a comment to indicate that the snippet is a type signature in f79420b

@feihong feihong requested a review from jchavarri December 11, 2024 17:52
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