Closed deiwin closed 8 years ago
Out of curiosity: what case made you do that? Do you really use that much html in your code? Technically I agree with this PR... but <h1>
? Seriously?
Well, this came about because I was trying to use markdown, similar to that in the test, in a @property
description. But that got rendered to faulty HTML, with every other HTML tag in place, but only the first <li>
tag being removed. I thought there's a bug with the MD rendering, but turns out the bug was with the stripping here. So I fixed it to avoid the confusion.
I will soon submit a PR for allowing full MD in @property
description as well, because I don't see any reason not to have this. And the fact that it doesn't support full MD also doesn't seem to be documented. There's another bug with the summary/description detection that has to be fixed first, though.
https://github.com/coffeedoc/codo#github-flavored-markdown - it is documented. It's mainly done to not end up with damaged layout (which can differ from theme to theme). Can I please ask you to provide a useful case where full-featured html could be useful in code-level documentation? Or particular case that rendered wrong for you. Just paste it here please.
This was my actual use case
class ThingRequest
# @property {Promise}
#
# ### Fulfilled with {Thing}
# If the request is successful and an the other party accepts the request then
# this promise will be fulfilled with the resulting {Thing}.
#
# ### Rejected with {Error}
# If the request is unsuccessful for any reason then this promise will be
# rejected with an {Error}. The {Error} may have one of the following causes:
#
# - INVALID_INPUT
# - DECLINED
# - UNAVAILABLE
# - CANCEL
# - TIMEOUT
# - INTERNAL_ERROR
thingPromise: null
As for the link to the README's GitHub Flavored Markdown section, yes I saw that part, but what I meant was that it doesn't list @property
.
So I've been thinking on this lately. This PR in general doesn't make sense at all. This kind of "limitation" doesn't really limit anything. Also you mention h1
in your comment but it's not even in the list of allowed tags. So basically this code is still supposed to strip it off. In other words your PR doesn't help the aforementioned comment to be converted directly. Am I missing something?
Basically if we go with supporting all the tags - we can just skip this limiter at all. But right now the UI isn't ready. From one side I understand that removing that would make things more scalable for users and hey, if UI is destroyed by incorrect tag one day - you just go and remove it. On the other side you are the first who actually requested that. What if we, instead, will allow to disable this filter via, say, a command key? I think this should cover all the needs.
In other words your PR doesn't help the aforementioned comment to be converted directly. Am I missing something?
I think you are misunderstanding the purpose of this PR, yes. This PR doesn't allow my example to be converted into full-fledged HTML, what it does is avoid creating faulty HTML. Let me give you a clear example of what this PR changes:
Say a Codo user, who hasn't read the documentation and doesn't know that they can't use full markdown for @property
documentation, has the following code (similar to the example I gave above) and generates documentation for it:
class Thing
# @property {Object}
# The property can be one of the following
# - OPTION1
# - OPTION2
prop: null
At the moment the generated HTML would include something like the following:
The property can be one of the following <ul>
OPTION1
<li>OPTION2</li>
</ul>
Which I hope you agree is unexpected and broken. Whereas the expected output, and the output with this PR being merged, would include something like the following:
The property can be one of the following
OPTION1
OPTION2
So this PR isn't trying to change any of the current behavior, and I'm not trying to convince you that you should enable unlimited Markdown in the @property
tag, for example. What this PR does is make sure that the limitations you currently already have in place behave correctly and don't generate faulty HTML (viz the missing <li>
tag above).
A-ha. Correct. Sorry, don't have to much time to dig deep into things lately. Very good then, thank you.
No worries :) Thanks for merging.
This fixes a few issues with the current solution, all of which are covered by the accompanied test:
(.+?)
to([\s\S]+?)
.<h1>
with the change from[a-z]
to[a-z0-9]
.<h1 id="title">
by adding\s*(?:\s[^>]+)?
after the tag name matcher.g
flag.g
flag redundant, but I left the flag in, because it avoids the recursive call stack getting unnecessarily deep.The test could probably be improved to not have to require the one trailing whitespace in and the empty newlines at the end of the expected string, but I couldn't think of a good way to do this right away.