v2.ocaml.org icon indicating copy to clipboard operation
v2.ocaml.org copied to clipboard

Bump OMD to 2.0.0~alpha1

Open patricoferris opened this issue 3 years ago • 20 comments

This issue is to upgrade the OCaml scripts which use Omd to the latest version of Omd. This issue is not necessarily very straight forward, but would expose someone to a lot of OCaml code, compiler errors and working in harmony with the type checker.

The main change is in the omd abstract syntax tree (AST) -- for example in scripts/code.ml we have:

match Omd.of_string ("<pre>" ^ p ^ "</pre>") with
    | [ Omd.Html_block(_,_,o) ] -> o
    | _ -> assert false

But the AST has changed, and Omd.of_string now returns a doc type (https://github.com/ocaml/omd/blob/master/src/omd.mli#L66) so this pattern-matching with the latest Omd returns an error.

Error: This pattern matches values of type Omd.block_desc
       but a pattern was expected which matches values of type Omd.block

So we need to convert instances like this to the new AST. For most cases this isn't too bad because the new AST simply wraps most things in a block type which has a block_desc and some attributes. The fix for this line becomes:

match Omd.of_string ("<pre>" ^ p ^ "</pre>") with
    | Omd.[{ bl_desc = Html_block o; _ }] -> o
    | _ -> assert false

To start developing a solution be sure to update your version of omd i.e. opam install omd.2.0.0~alpha1.

patricoferris avatar Mar 30 '21 11:03 patricoferris

Hi @patricoferris I haven't taken any issues yet to solve, do you think this would be a good start? 😃

CarolVilarino avatar Mar 31 '21 18:03 CarolVilarino

HI @CarolVilarino I'm not sure this is a good first issue -- there's quite a lot of OCaml code to change and this repository doesn't have the dune build system which makes it even more challenging to update. I think you have commented on some other issues so hopefully you have another one?

patricoferris avatar Apr 01 '21 09:04 patricoferris

@patricoferris can I try working on this?

Srinithyee avatar Apr 08 '21 04:04 Srinithyee

Hi @Srinithyee I've done a little extra digging on this issue, so let me explain some things and then you can see if you still want to work on it.

The latest omd binary doesn't have the same level of functionality as the old one. In particular, it cannot generate table of contents like we do now (i.e. omd -otoc...), so until that functionality is brought back we cannot upgrade to omd.2.0.0~alpha1 without rewriting that ourselves, but there are plans to add it back in perhaps (see https://github.com/ocaml/omd/issues/232).

However, we can still work the the omd library (the parts I was describing above). We can convert to the new AST and make sure that the scripts compile and then leave that as a draft PR ready to be merged if/when the omd binary gets the support we need.

In summary, the work done on this won't be merged probably any time soon but it is still a contribution I believe you can write about in your Outreachy app :)) Let me know what you think, thanks.

patricoferris avatar Apr 09 '21 15:04 patricoferris

Just seen you are working on https://github.com/ocaml/ocaml.org/issues/1492 -- it's best that you focus on that one I think. If anyone else wants to work on this given the above let me know :))

patricoferris avatar Apr 09 '21 15:04 patricoferris

@Goodiec Since you were asking for a more challenging issue, you can take this one! :)

gs0510 avatar Apr 09 '21 17:04 gs0510

Just seen you are working on #1492 -- it's best that you focus on that one I think. If anyone else wants to work on this given the above let me know :))

@patricoferris Yes, I'd like to work on #1492. Thanks a lot for your clarification on this issue though :))

Srinithyee avatar Apr 10 '21 03:04 Srinithyee

@Goodiec Since you were asking for a more challenging issue, you can take this one! :)

OK @gs0510, I'll attempt this. Thank you.

Goodiec avatar Apr 10 '21 04:04 Goodiec

@Goodiec Are you still working on this issue? @gs0510 @patricoferris, If @Goodiec confirms she is not working on this Issue, Can I take this up?

jyotibalodhi avatar Apr 17 '21 20:04 jyotibalodhi

@Goodiec Are you still working on this issue? @gs0510 @patricoferris, If @Goodiec confirms she is not working on this Issue, Can I take this up?

Hi @jyotibalodhi, yes I am.

Goodiec avatar Apr 17 '21 21:04 Goodiec

@Goodiec have you been able to make any progress? Do you need any help? Thanks!

gs0510 avatar Apr 23 '21 09:04 gs0510

@Goodiec have you been able to make any progress? Do you need any help? Thanks!

Hello @gs0510, sorry this has taken longer than anticipated, been a bit sick. Please could you explain this issue better, I'm not very clear on how to go about it. Thank you

Goodiec avatar Apr 23 '21 12:04 Goodiec

No worries @Goodiec. Did you read the first comment on the issue here: https://github.com/ocaml/ocaml.org/issues/1321#issue-844401468 :)

omd is a library that the code depends on and the code doesn't compile with the newest version of omd which is 2.0.0~alpha1. In the comment, Patrick has also mentioned how you can fix one of the instances where the code fails. Can you go through it and let me know what you don't understand? Thanks!

gs0510 avatar Apr 23 '21 13:04 gs0510

No worries @Goodiec. Did you read the first comment on the issue here: #1321 (comment) :)

omd is a library that the code depends on and the code doesn't compile with the newest version of omd which is 2.0.0~alpha1. In the comment, Patrick has also mentioned how you can fix one of the instances where the code fails. Can you go through it and let me know what you don't understand? Thanks!

Yes, I did. What I gathered from the issue description is that I have to make some syntax changes to match the latest omd library in the affected places in the scripts but I wasn't sure. I will open a pull request on this soon and get validation that I am doing the right thing. Thank you @gs0510.

Goodiec avatar Apr 23 '21 19:04 Goodiec

Yes that sounds about right :) Thanks @Goodiec! :)

gs0510 avatar Apr 24 '21 09:04 gs0510

Hello @patricoferris, @gs0510 . After installing the new Omd version, I went ahead to make some changes based on the example given above but trying to build the site using make local gives me the error in the image below:

err

Please advise on this.

Goodiec avatar Apr 26 '21 12:04 Goodiec

Hi @Goodiec, awesome! I've just been talking about this work as the Omd team look to do a new major release. Absolutely no pressure, but just letting you know your work will be super useful !

This is a compiler error, in particular a type error. You have an if a then b else c statement. The type of a is a bool, the types of b and c could be anything, but importantly they must be the same. Consider:

print_endline (if true_or_false then "Hello" else "World")

print_endline takes a string and prints it, so the clauses of the if statement better both be the same type. In this case the compiler says:

This expression has type 'weak1 list but an expression was expected of type string

Ignoring the 'weak1 part for now, the problem is that the first clause of the this function is returning a string and the second clause is returning a list. They should be the same type.

I would recommend having a look at the old Omd interface and trying to understand how things map to the latest version.

It is also important to work out what each function is trying to do and map that to the new version of Omd. So for this function that is causing the type error, the code is trying to highlight some OCaml code and return the generated HTML which is the Omd.t which no longer exists, I would suggest having a look at Omd.doc type https://github.com/ocaml/omd/blob/master/src/omd.mli#L61.

Please be aware that this is not an easy issue at all, migrating to the new AST is not straightforward so please let me know if you encounter any other uncertainties.

patricoferris avatar Apr 26 '21 15:04 patricoferris

Hi @Goodiec, awesome! I've just been talking about this work as the Omd team look to do a new major release. Absolutely no pressure, but just letting you know your work will be super useful !

This is a compiler error, in particular a type error. You have an if a then b else c statement. The type of a is a bool, the types of b and c could be anything, but importantly they must be the same. Consider:

print_endline (if true_or_false then "Hello" else "World")

print_endline takes a string and prints it, so the clauses of the if statement better both be the same type. In this case the compiler says:

This expression has type 'weak1 list but an expression was expected of type string

Ignoring the 'weak1 part for now, the problem is that the first clause of the this function is returning a string and the second clause is returning a list. They should be the same type.

I would recommend having a look at the old Omd interface and trying to understand how things map to the latest version.

It is also important to work out what each function is trying to do and map that to the new version of Omd. So for this function that is causing the type error, the code is trying to highlight some OCaml code and return the generated HTML which is the Omd.t which no longer exists, I would suggest having a look at Omd.doc type https://github.com/ocaml/omd/blob/master/src/omd.mli#L61.

Please be aware that this is not an easy issue at all, migrating to the new AST is not straightforward so please let me know if you encounter any other uncertainties.

Thank you @patricoferris, I will go through the resources you provided and let you know if I encounter any difficulty or have further questions.

Goodiec avatar Apr 26 '21 15:04 Goodiec

Hello @patricoferris, I have been getting more familiar with Ocaml to help me better understand the functions I am to make changes to. Please could you provide me with a resource or resources that could help me understand the Omd interfaces better as I am still having blockers and there is no proper documentation on the library, I really want to work on this issue.

Goodiec avatar May 05 '21 14:05 Goodiec

As with most OCaml libraries the documentation is sparse. The best bet is to get comfortable with reading .mli files which describe the interfaces and also using the library. For example, you can open utop -require omd and start parsing strings to see the generated AST.

utop # Omd.of_string "# Hello World";;
- : Omd.doc =
[{Omd.bl_desc =
   Omd.Heading (1, {Omd.il_desc = Omd.Text "Hello World"; il_attributes = []});
  bl_attributes = []}]

Unfortunately that's the nature of programming in OCaml, as you get more and more familiar with common practices the .mli files actually end up being very useful indeed, not a complete replacement for written documentation, but 8/10 they are enough to get going. If you have any specific questions though, feel free to ask them here :))

patricoferris avatar May 06 '21 08:05 patricoferris