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

[BUGFIX] Adds support for decorators #52

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Oct 30, 2019

This PR adds general support for decorators when used within markdown blocks.
It does this by reaching into the YUIDoc global and patching the method that
handles comment parsing. Given YUIDoc is pretty much abandoned at this point,
this should be a relatively stable patch. Presumably, attempts to modernize it
will also add decorator support, should they occur.

The fix replaces all @ symbols within codeblocks with a placeholder, processes
them, and then switches them back after processing.

This PR adds general support for decorators when used within markdown blocks.
It does this by reaching into the YUIDoc global and patching the method that
handles comment parsing. Given YUIDoc is pretty much abandoned at this point,
this should be a relatively stable patch. Presumably, attempts to modernize it
will also add decorator support, should they occur.

The fix replaces all @ symbols within codeblocks with a placeholder, processes
them, and then switches them back after processing.
@cibernox
Copy link
Owner

LGTM.

@rwjblue
Copy link
Contributor

rwjblue commented Oct 30, 2019

@pzuraq - don’t we need to preserve the known set of YUIDoc pragma’s? Things like @example, @method, @returns, etc...

Oops, I misread the diff, sorry. This only replaces @ within code fences. I think we also need to deal with code blocks not in triple backticks (you can indent exactly 4 spaces from the normal comment indentation and also be considered a code block). Though I don’t know how common this is in Ember...

@okuryu
Copy link
Collaborator

okuryu commented Oct 30, 2019

Is this a problem that YUIDoc should fix?

@cibernox
Copy link
Owner

@rwjblue If you think this can be merged as is, I'll merge it and release a new version today

@pzuraq
Copy link
Contributor Author

pzuraq commented Oct 30, 2019

@okuryu unfortunately YUIDoc is a dead project (hasn’t merged a commit in 4years or so now)

Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Ya, this fix looks good to me. We could do a follow up to add support for the indented format, but I kinda doubt we use it much (since we use the code fences to invert type, file name, etc).

@okuryu
Copy link
Collaborator

okuryu commented Oct 31, 2019

@okuryu unfortunately YUIDoc is a dead project (hasn’t merged a commit in 4years or so now)

Yes. If this is a bug that should be fixed in YUIDoc, I thought I could cooperate.

@pzuraq
Copy link
Contributor Author

pzuraq commented Oct 31, 2019

Ah, didn't realize you were a maintainer! Let me see if I can adapt my fix to the existing code, would love to fix this upstream!

@okuryu
Copy link
Collaborator

okuryu commented Oct 31, 2019

Thanks. If you are in a hurry to fix this issue, please free to merge this PR. Fixing the upstream are also welcomed. I think I can take a look too.

@pzuraq
Copy link
Contributor Author

pzuraq commented Oct 31, 2019

Yeah, I think this would still be good to merge, since the upstream fix may require more changes (for instance to handle indented markdown examples, or @example examples). This unblocks us locally, but I'll definitely make a PR upstream as well to at least add this functionality, and try to take a stab at more general fixes as well.

@cibernox cibernox merged commit 02a2413 into cibernox:master Oct 31, 2019
@cibernox
Copy link
Owner

Published in 0.9.0

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.

4 participants