-
Notifications
You must be signed in to change notification settings - Fork 124
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
Inline puml #1100
Inline puml #1100
Conversation
aa7eb37
to
05ce1a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty neat 👍, lets wait for some comments
src/util/pluginUtil.js
Outdated
} | ||
|
||
if (tagAttribs.src !== undefined) { | ||
const fileName = fsUtil.removeExtension(tagAttribs.src); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about adding path.basename(tagAttribs.src)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we want to have it as fsUtil.removeExtension
. For example, if the src='diagrams/activity.puml
, this way the output fileName will be src='diagrams/activity.png'
.
With path.basename(tagAttribs.src)
, it becomes activity.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I meant fsUtil.removeExtension(path.basename(tagAttribs.src))
, but I realised omitting basename keeps the output file in the same directory as the input file.
In that case how about changing fileName
-> filePath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the behaviour when src
is specified consistent, if a name
is also specified,
should we instead return the file path that is the result of replacing the basename of src
with name
?
src/util/pluginUtil.js
Outdated
/** | ||
* Returns the string content of the plugin. | ||
*/ | ||
getPluginContent: ($, element, cwf, config) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we expand the scope of this PR a little?
-
I realised
cwf
will always be available after refactoringcomponentPreprocessor
, let's removeconfig
from the entire plantuml plugin, and consequently the entireconfig
( I don't see any other plugins using this, but you could double confirm ). This should be a good step for Support always-on plugins #714 (comment) ( which is no longer necessary with the prior refactors ). -
We currently don't have any plugins that use
config
save for this anymore, so let's repurpose it for plugin utils, as callingconst pluginUtil = require('../../util/pluginUtil');
on every plugin is dependent on the plugin's location. E.g. if the plugin is inmarkbind
, one would have to knowpluginUtil
's location relative to the parent module or in the file system.- Let's then expose these pluginUtils. Since we only have one plugin using this now, we can leave it undocumented for a while until we have more and the implementation / requirements becomes more final.
I think its a good step to rectify the solution that was used in #714 (comment), and since pluginUtils
as I see it here isn't parser/page dependent, I think exposing it eventually after a few more rounds of plugin development would be a good option. Might be good to have @marvinchin 's opinion since he was involved though 😁.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, could you elaborate more on point 2? Not too sure I am following well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking of
preRender: (content, pluginContext, frontMatter, useful safe interfaces (pluginUtil) that we want to expose to plugins) => ...
instead of depending on
preRender: (content, pluginContext, frontMatter, secret unsafe details) => ...
or having to rely on require('../../util/pluginUtil')
where the path may differ from plugin to plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. However, this introduces a breaking change if we remove config from pre/post-render, especially if there are users who have defined their own plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was intentionally undocumented because it exposed too much implementation details #714 (comment) ( the above comment ); should be fine here
05ce1a2
to
aac7d2b
Compare
@ang-zeyu thanks for your review! Thanks for pointing out that cwf always exist, I refactored slightly to make the code simpler. I also added the changes to user guide. Currently I would like to scope this PR smaller! Wanted to point out that PlantUML plugin is currently still using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently I would like to scope this PR smaller!
Ok with this as well 👍, since the usage from the author's side wouldn't change
I realised the old code also does not deal with the case that src
uses {{ baseUrl }}
, which is similar to what #1088 fixes for includes. This can be included in a separate PR though, since it'll likely need a supporting interface from pluginUtils
.
Couple more nits otherwise:
src/util/pluginUtil.js
Outdated
} | ||
|
||
if (tagAttribs.src !== undefined) { | ||
const fileName = fsUtil.removeExtension(tagAttribs.src); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I meant fsUtil.removeExtension(path.basename(tagAttribs.src))
, but I realised omitting basename keeps the output file in the same directory as the input file.
In that case how about changing fileName
-> filePath
?
src/util/pluginUtil.js
Outdated
} | ||
|
||
if (tagAttribs.src !== undefined) { | ||
const fileName = fsUtil.removeExtension(tagAttribs.src); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the behaviour when src
is specified consistent, if a name
is also specified,
should we instead return the file path that is the result of replacing the basename of src
with name
?
aac7d2b
to
65177a5
Compare
@ang-zeyu thanks for your review! Indeed filePath would've been more appropriate, updated accordingly. 👍
I thought a little about this. However, I believe letting <puml name="output/activity">
activity diagram
</puml>
<puml name="output/object" src="input/object.puml" /> Here, an author would like both output to be in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍; works well too
Just one really minor nit left:
65177a5
to
6f13ed6
Compare
Hi, thanks for the changes. Are you able to ping for reviewers in this repo? |
@marvinchin, could you help to review this? Thanks 🙇♂️ ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did another round of testing, looks good! 👍
Hi Chun Rong @crphang It seems that updating tests on Windows |
* 'master' of https://github.com/MarkBind/markbind: (41 commits) Document adding new site content in DG (MarkBind#1153) Add relative date feature (MarkBind#908) Use <br> to separate lines of code block (MarkBind#1176) Parse popovers for footnotes (MarkBind#1155) Resolve comments Expand test extensions and fix whitespace checks (MarkBind#1156) Address comments Upgrade js-beautify and provide option to turn it off (MarkBind#1116) Normalize inline puml line ending before hashing (MarkBind#1174) Update tests (MarkBind#1178) Remove fixed bugs from test\functional\test_site\bugs\index.md` (MarkBind#1148) Fix bug in Search for UG and DG (MarkBind#1147) Add inline puml support (MarkBind#1100) Fix hrefs for headings with angular brackets (MarkBind#1089) Update tests for 2.13.1 (MarkBind#1169) 2.13.1 Update vue-strap version to v2.0.1-markbind.39 Fix fontawsome icons don't show underlines to indicate modal/tooltip (MarkBind#1133) 2.13.0 Update test files ...
Seems to be working in production. Good work @crphang and reviewers. |
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] New feature
Resolves #912, continuation of #968.
What is the rationale for this request?
Added inline PUML support which allows user to write PUML inline.
What changes did you make? (Give an overview)
puml
as a special tag which allows markdown-it to ignore the content (escaping it accordingly).Provide some example code that this change will affect:
The following tag would produce puml documents.
Is there anything you'd like reviewers to focus on?
This feature relies on special tags and would require #1099 to be resolved enabling having multiple puml tags:
Switching PUML as a special tag now would break existing behavior. To illustrate this: I have switched all puml tags in tests to: