add preprocess support for PlantUML "!include" #75

anb0s · 2020-05-16T14:29:52Z

Fixes #49

see:
http://plantuml.com/en/preprocessing
http://plantuml.com/en/commons

implement and test:

anb0s · 2020-05-16T15:26:28Z

@Mogztter please review the first naive implementation ;)

src/preprocess.js

ggrossetie · 2020-05-17T08:11:47Z

src/preprocess.js

+      let vfs = context.vfs
+      if (typeof vfs === 'undefined' || typeof vfs.read !== 'function') {
+        vfs = require('./node-fs')
+      }


I will try to come up with a solution to hide this idiom

Still use this additionally with exists function

src/preprocess.js

ggrossetie · 2020-05-17T08:22:20Z

reinclude protection (recursive include loop)

That's a good point but with your current implementation we do not resolve include recursively right? I mean we only resolve the first level of include no?
We should add a test on this case:

main.puml -> !include style.puml -> !include color.puml

And we should read PlantUML code to see how they prevent recursive include loop.

anb0s · 2020-05-17T12:24:15Z

reinclude protection (recursive include loop)

That's a good point but with your current implementation we do not resolve include recursively right? I mean we only resolve the first level of include no?
We should add a test on this case:
main.puml -> !include style.puml -> !include color.puml
And we should read PlantUML code to see how they prevent recursive include loop.

Yes it's not implemented and is a hint for the future. I will add this feature and check how to prevent include loop.

ggrossetie · 2020-05-22T23:04:55Z

@anb0s I see that you are making progress! Let me know when I can review it again. I've merged #42 in anticipation because once every includes will be inlined we will probably exceed the URI limit more often.
Anyway, keep up the good work 👍

anb0s · 2020-05-23T15:07:08Z

@Mogztter please review the changes :)

anb0s · 2020-05-23T15:09:48Z

Some tests related to "HTTP client / Adaptive mode" and "Conversion" are now failing and it looks not related to my last commit - i'm sure it worked some hours before ;)

ggrossetie · 2020-05-23T15:22:22Z

Some tests related to "HTTP client / Adaptive mode" and "Conversion" are now failing and it looks not related to my last commit - i'm sure it worked some hours before ;)

I've just released and published a new version of Kroki and some images are not exactly the same with the latest version. We need to update the expected output accordingly.

ggrossetie · 2020-05-24T08:35:58Z

@danyill just fixed the tests, please rebase on master.

anb0s · 2020-05-24T17:33:03Z

I've thought about some issues not yet adressed:

@Mogztter : you have metioned trailing comments: # this is a comment but i didn't found any documentation about it
!include in comments: e.g. single line '!include my-file.iuml' and multiline /' ... !include my-file.iuml ... '/ are processed, but must be skipped
including whole file that has @startuml ... @enduml block(s) --> check how original PlantUML handles them: remove them from file or just include the whole file and PlantUML will complane at server? We are using always *.iuml files without any @startuml..@enduml blocks, so it works for us ;)
we just include one file multiple times except if's not included cyclic (prevent infinite include loop), how is about new/old preprocessor:

!include now allows multiple inclusions : you don't have to use !include_many anymore
...
Note that there is also a !include_once directive that raises an error if a file is included several times.

--> should we support !include_many and !include_once ?

ggrossetie · 2020-05-24T21:38:13Z

should we support !include_many and !include_once ?

Good catch! I wasn't even aware of these two directives 🤯
I've checked and they are still supported in the latest version of PlantUML so, unfortunately, we need to support them as well 😬

you have mentioned trailing comments: # this is a comment but i didn't found any documentation about it

Yes, it's not documented but you can give it a try: http://www.plantuml.com/plantuml/uml/ZOz1ImCn48Nl-HLnlQnWDmezUQginwgbwbdIxD0Dp4x2p0JflpSkw2AYU7elWVUzDnjHprecD6UH0fO12gCTLRFSECV-hJj67SkX2718f43QXJIvVGoUX_GG8GEwVhtQeITzkTyw1WGluAvNL_N_jQyX7mcuoxN5djj-RJ-SCJjWITJcMMunUhcqpTOV09jg7hF6H1xj7KZWEB-Q2_E5QyEd-n5zXZ1ImdGy_G5wEed2rCei_sqtUy3lWtzTiI2MEPMm0HhgzUy0

!include in comments: e.g. single line '!include my-file.iuml' and multiline /' ... !include my-file.iuml ... '/ are processed, but must be skipped

it should be relatively easy, I think that an include directive is only valid if it starts with 0 or more spaces, otherwise it won't get processed. So, the following are valid

   !include /path/to/something.puml
!include /path/to/something.puml
 !include /path/to/something.puml

But the following are not:

a   !include /path/to/something.puml
\!include /path/to/something.puml
 '!include /path/to/something.puml

Multi-line comments are more complex to handle because we will need to ignore content when we find /' until we find '/.

including whole file that has @startuml ... @enduml block(s) --> check how original PlantUML handles them: remove them from file or just include the whole file and PlantUML will complane at server? We are using always *.iuml files without any @startuml..@enduml blocks, so it works for us ;)

I think it should work, PlantUML will ignore/remove them.

I will try to take some time this week to review this great contribution 👍

anb0s · 2020-05-25T14:53:52Z

hm, i've tried all the files i've created for tests with original PlantUML and found that !include_once is not an include guard in the included file itself like i've implemented, but it's just like !include_many my-file.iuml and !include my-file.iuml you can use !include_once my-file and than it will produce error if used multiple times:

produces error:

@startuml
  !include_once test\fixtures\plantuml\style-general.iuml
  alice -> bob
  !include_once test\fixtures\plantuml\style-general.iuml
@enduml

no error:

@startuml
  !include_once test\fixtures\plantuml\style-general.iuml
  alice -> bob
  !include test\fixtures\plantuml\style-general.iuml
@enduml

That's should be easy to change...

anb0s · 2020-05-25T15:14:00Z

hm, i've tried all the files i've created for tests with original PlantUML and found that !include_once is not an include guard in the included file itself like i've implemented, but it's just like !include_many my-file.iuml and !include my-file.iuml you can use !include_once my-file and than it will produce error if used multiple times:

produces error:
@startuml
  !include_once test\fixtures\plantuml\style-general.iuml
  alice -> bob
  !include_once test\fixtures\plantuml\style-general.iuml
@enduml
no error:
@startuml
  !include_once test\fixtures\plantuml\style-general.iuml
  alice -> bob
  !include test\fixtures\plantuml\style-general.iuml
@enduml
That's should be easy to change...

Done!

anb0s · 2020-05-25T15:18:50Z

To be specified: !include local-file-path-or-url should remove @startuml and @enduml lines from included file and include only first block (index 0) or include all blocks as one?

Tested with original PlantUML and adapted requirement:
!include local-file-path-or-url if included file has @startuml ... @enduml block(s) it should include only first block (index 0)

anb0s · 2020-05-25T15:39:57Z

To be specified: !include local-file-path-or-url should remove @startuml and @enduml lines from included file and include only first block (index 0) or include all blocks as one?

Tested with original PlantUML and adapted requirement:
!include local-file-path-or-url if included file has @startuml ... @enduml block(s) it should include only first block (index 0)

Done!

anb0s · 2020-05-25T15:42:29Z

From my point of view the important !include features are implemented and tested.

Please review...

src/asciidoctor-kroki.js

src/preprocess.js

Fixes asciidoctor#49 see: http://plantuml.com/en/preprocessing http://plantuml.com/en/commons implement and test: - [x] `!include local-file-path` (throw an error if file not found) - [x] `!include local-file-path-with-spaces` - [x] `!include ... # comment` - [x] `!include remote-file-url` (skip and warn if url not accessible) - [x] `!include <std/lib/path>` (skip and warn) - [x] `!include file!index` - [x] `!include file!id` - [x] `!includeurl url` (compatibility : same as `!include remote-file-url`) - [x] `!includesub id` (compatibility : includes `!startsub id ... !endsub`) - [x] recursive include - [x] cyclic include loop protection (throw an error if detected) - [x] remove block `/' ... '/` and line `' ... '` comments - [x] `!include_many local-file-or-url` (compatibility : same as normal `!include ...`) - [x] `!include_once local-file-or-url` (throw an error if file included multiple times) - [x] `!include local-file-path-or-url` if included file has `@startuml ... @enduml` block(s) it should include only first block (index 0)

ggrossetie · 2020-05-29T22:15:37Z

I will probably checkout your branch tomorrow and carefully review the code 👀

ggrossetie · 2020-05-31T15:52:35Z

@anb0s Overall this is really good and I've made only a few tiny changes. The biggest change is that the preprocessor now preserves comments. I think it's important otherwise we will "lose" information if we want to use the encoded format (ie. the URL) as the "source of trust".
I don't recommend it but some users asked me if all the content was in the encoded format so I think it's important to preserve comments.
I've also added a few tests (but most of the cases were already covered 👍) and I also resolve the "first" !include directive relative to the base directory instead of .. For instance:

[plantuml]
....
!include ./style-general.iuml
alice -> bob
....

If you base directory is /path/to/docs, the preprocessor will try to read /path/to/docs/style-general.iuml. If base directory is undefined, I believe that the value will be "the current working directory".

Let me know what you think.

anb0s · 2020-05-31T18:52:29Z

@anb0s Overall this is really good and I've made only a few tiny changes. The biggest change is that the preprocessor now preserves comments. I think it's important otherwise we will "lose" information if we want to use the encoded format (ie. the URL) as the "source of trust".
I don't recommend it but some users asked me if all the content was in the encoded format so I think it's important to preserve comments.

I've also thought first to preserve them, but then decided as not needed and easier to remove instead of "handle". So i'm fine with preserving comments 👍

I've also added a few tests (but most of the cases were already covered 👍) and I also resolve the "first" !include directive relative to the base directory instead of ..

Yes, that's also good to have it explicitly defined instead of expecting working directory be the same as base directory of the document.

If you base directory is /path/to/docs, the preprocessor will try to read /path/to/docs/style-general.iuml. If base directory is undefined, I believe that the value will be "the current working directory".

Yes it looks like it's the current working directory if not defined.

Let me know what you think.

I've reviewed the changes and it looks good to me. I can maybe try it tomorrow, but if all tests are green i'm fine with this 💯

Thank You!

anb0s · 2020-06-01T08:16:48Z

I've checked out the new version and it looks good to me!

From my point of view it can be merged ;)

ggrossetie · 2020-06-01T17:27:00Z

Let's do this!
A big thanks for your work Andre 👍

anb0s force-pushed the plantuml-include branch from 601c06e to 82e54d3 Compare May 16, 2020 15:18

ggrossetie reviewed May 17, 2020

View reviewed changes

src/preprocess.js Outdated Show resolved Hide resolved

ggrossetie reviewed May 17, 2020

View reviewed changes

src/preprocess.js Outdated Show resolved Hide resolved

anb0s marked this pull request as draft May 17, 2020 11:48

anb0s changed the title ~~add basic preprocess support for PlantUML "!include"~~ [WIP] add basic preprocess support for PlantUML "!include" May 17, 2020

anb0s force-pushed the plantuml-include branch 3 times, most recently from 40f098f to 145f606 Compare May 22, 2020 22:39

anb0s force-pushed the plantuml-include branch from 145f606 to e0337a1 Compare May 22, 2020 23:20

anb0s marked this pull request as ready for review May 22, 2020 23:21

anb0s force-pushed the plantuml-include branch 4 times, most recently from 8012cc4 to 99726cc Compare May 23, 2020 15:02

anb0s changed the title ~~[WIP] add basic preprocess support for PlantUML "!include"~~ add basic preprocess support for PlantUML "!include" May 23, 2020

anb0s force-pushed the plantuml-include branch from 99726cc to 850c9ea Compare May 23, 2020 18:44

danyill mentioned this pull request May 23, 2020

Update tests for new kroki release #82

Closed

anb0s force-pushed the plantuml-include branch from 850c9ea to c33e984 Compare May 24, 2020 10:16

anb0s force-pushed the plantuml-include branch from c33e984 to 1bd0ff2 Compare May 24, 2020 22:00

anb0s force-pushed the plantuml-include branch 2 times, most recently from 48a7c64 to 630d101 Compare May 25, 2020 12:19

anb0s changed the title ~~add basic preprocess support for PlantUML "!include"~~ add preprocess support for PlantUML "!include" May 25, 2020

anb0s force-pushed the plantuml-include branch from 630d101 to 059e2f2 Compare May 25, 2020 15:07

anb0s force-pushed the plantuml-include branch from 059e2f2 to e711779 Compare May 25, 2020 15:37

danyill mentioned this pull request May 25, 2020

Add support of PlantUML asciidoc extention asciidoctor/atom-asciidoc-preview#122

Open

ggrossetie reviewed May 25, 2020

View reviewed changes

src/asciidoctor-kroki.js Outdated Show resolved Hide resolved

ggrossetie reviewed May 25, 2020

View reviewed changes

src/preprocess.js Outdated Show resolved Hide resolved

ggrossetie reviewed May 26, 2020

View reviewed changes

src/preprocess.js Outdated Show resolved Hide resolved

anb0s force-pushed the plantuml-include branch from e711779 to 6253ca4 Compare May 26, 2020 17:53

anb0s force-pushed the plantuml-include branch from 6253ca4 to 8f71c5b Compare May 26, 2020 18:58

ggrossetie added 6 commits May 31, 2020 15:11

Require node-fs only if necessary

e0b2245

Preserve comments

19b1d28

Add a test to make sure that include is resolved relative to the file

5f510ff

Use the base directory to resolve PlantUML relative include path

5940b97

Generate dist

68d2db0

Remove final newline in fixtures

698e120

ggrossetie force-pushed the plantuml-include branch from 7a95400 to 698e120 Compare May 31, 2020 15:38

ggrossetie merged commit 1c91c83 into asciidoctor:master Jun 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add preprocess support for PlantUML "!include" #75

add preprocess support for PlantUML "!include" #75

anb0s commented May 16, 2020 •

edited

anb0s commented May 16, 2020

ggrossetie May 17, 2020

anb0s May 24, 2020

ggrossetie commented May 17, 2020

anb0s commented May 17, 2020

ggrossetie commented May 22, 2020

anb0s commented May 23, 2020

anb0s commented May 23, 2020

ggrossetie commented May 23, 2020

ggrossetie commented May 24, 2020

anb0s commented May 24, 2020 •

edited

ggrossetie commented May 24, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

ggrossetie commented May 29, 2020

ggrossetie commented May 31, 2020

anb0s commented May 31, 2020

anb0s commented Jun 1, 2020

ggrossetie commented Jun 1, 2020

add preprocess support for PlantUML "!include" #75

add preprocess support for PlantUML "!include" #75

Conversation

anb0s commented May 16, 2020 • edited

anb0s commented May 16, 2020

ggrossetie May 17, 2020

Choose a reason for hiding this comment

anb0s May 24, 2020

Choose a reason for hiding this comment

ggrossetie commented May 17, 2020

anb0s commented May 17, 2020

ggrossetie commented May 22, 2020

anb0s commented May 23, 2020

anb0s commented May 23, 2020

ggrossetie commented May 23, 2020

ggrossetie commented May 24, 2020

anb0s commented May 24, 2020 • edited

ggrossetie commented May 24, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

anb0s commented May 25, 2020

ggrossetie commented May 29, 2020

ggrossetie commented May 31, 2020

anb0s commented May 31, 2020

anb0s commented Jun 1, 2020

ggrossetie commented Jun 1, 2020

anb0s commented May 16, 2020 •

edited

anb0s commented May 24, 2020 •

edited