search

jelly-beam / rebar3_ex_doc

rebar3 plugin for generating docs with ex_doc
Apache License 2.0
43 stars 13 forks source link

Fix `with_mermaid` #64

Closed MarkoMin closed 7 months ago

MarkoMin commented 1 year ago

Although it seems like an easy problem, it turned out to be pretty nasty. But apart from solving this particular problem, 2 more problems are solved:

  1. ex_doc seemed to ignore multiple "before_closing_body_tag"s, so in previous solution it wasn't possible to inject custom html while using with_mermaid option.
  2. Mermaid script element wasn't closed properly so browser ended up merging those 2 script elements effectively ignoring the whole event listener.

Tried my best, but didn't come up with an elegant solution, but it works. Suggestions are very welcome!

Closes #63

starbelly commented 1 year ago

@MarkoMin Thank you for your patience, I have left a few initial comments / questions. Could you also either a add test or explain how to test the cases you are interested in?

MarkoMin commented 1 year ago

I would like to set a test case, but I didn't have an idea on how to write one, so I tested the solution manually. We need to test all combinations of with_mermaid and before_closing_body_tag to ensure they all get merged properly and then look at resulting HTML and see if mermaid HTML is placed before before_closing_body_tag HTML. So, we'd prolly need some HTML parsing stuff

starbelly commented 1 year ago

HI, it's taken a bit of time to get this, so I appreciate your patience.

I would like to set a test case, but I didn't have an idea on how to write one, so I tested the solution manually. We need to test all combinations of with_mermaid and before_closing_body_tag to ensure they all get merged properly and then look at resulting HTML and see if mermaid HTML is placed before before_closing_body_tag HTML. So, we'd prolly need some HTML parsing stuff

So, it's not terribly hard, but it's some what non-obvious, and there's not much in the way of helpers either, such that I haven't been super strict about people adding tests. That said, have a look at : https://github.com/starbelly/rebar3_ex_doc/blob/b04392cf17a53141f7ccd4f17af635998398866a/test/rebar3_ex_doc_SUITE.erl#L31

You can see we have check_docs/3 function called (by other tests as well), which can serve as an example as to how you would check the output.

As stated, it is a little cumbersome right now, and some of it requires knowledge about rebar_state, etc. and a lot of the validating of html would have to be done with regex right now (i.e., read in the file, perform a regex on the string and so forth) as we do not have an html parser in the mix right now.

I'm totally okay with checking this manually on this go around, but if you're interested in writing a test, I'm quite happy with that too 😄

MarkoMin commented 1 year ago

I'll try it next week, I also prefer having tests, especially in situation like this where solution is not elegant

starbelly commented 1 year ago

I'll try it next week, I also prefer having tests, especially in situation like this where solution is not elegant

If some tests are written, and then we can assert that the implementation is correct, then we can make it beautiful 😄

starbelly commented 9 months ago

We're going to move this repo to jelly-beam org soon, this will have be re-opened after that move, assuming we don't merge it before.

MarkoMin commented 8 months ago

Seems like passing configurations with {with_mermaid, true} and/or {before_closing_body_tag, ...} to make_stub/1 in tests have no effect, and I have no clue why. Resulting files (at least index.html) have no mermaid nor custom html included, while they should. Any clue why this is the case?

starbelly commented 8 months ago

Seems like passing configurations with {with_mermaid, true} and/or {before_closing_body_tag, ...} to make_stub/1 in tests have no effect, and I have no clue why. Resulting files (at least index.html) have no mermaid nor custom html included, while they should. Any clue why this is the case?

I think you want to use lists:keystore/4 vs lists:keyreplace/4, the latter will have no effect if the key already in the list of tuples (proplist). That at least would explain the {with_mermaid, true} case. The javascript would also not be injected into index.html, rather readme.html, any extras defined, module docs, etc.

MarkoMin commented 8 months ago

You're right, thanks! I've wrote a test that tests everything that is fixed in this PR. ATM I see no benefit into splitting it into separate tests. It's not state of the art, but hopefully is good enough

paulo-ferraz-oliveira commented 7 months ago

@starbelly, is this Ok to merge, for you?

starbelly commented 7 months ago

LGTM!