ocaml / v2.ocaml.org

Implementation of the ocaml.org website.
http://ocaml.org
Other
323 stars 346 forks source link

Bump OMD to 2.0.0~alpha1 #1321

Open patricoferris opened 3 years ago

patricoferris commented 3 years ago

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.

CarolVilarino commented 3 years ago

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

patricoferris commented 3 years ago

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?

Srinithyee commented 3 years ago

@patricoferris can I try working on this?

patricoferris commented 3 years ago

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 commented 3 years ago

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 :))

gs0510 commented 3 years ago

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

Srinithyee commented 3 years ago

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 :))

Goodiec commented 3 years ago

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

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

jyotibalodhi commented 3 years ago

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

Goodiec commented 3 years ago

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

gs0510 commented 3 years ago

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

Goodiec commented 3 years ago

@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

gs0510 commented 3 years ago

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!

Goodiec commented 3 years ago

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.

gs0510 commented 3 years ago

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

Goodiec commented 3 years ago

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.

patricoferris commented 3 years ago

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.

Goodiec commented 3 years ago

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 commented 3 years ago

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.

patricoferris commented 3 years ago

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 :))