-
Notifications
You must be signed in to change notification settings - Fork 109
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
Includes wrong diagram #258
Comments
Scratching my head on this one. I'll try to reproduce the problems your describing and let you know what I find. Regarding
that's a curious way to do this. With this syntax, asciidoctor core is processing the You can use the block macro to achieve the same result with fewer moving parts:
Specifying the target image name on a diagram block is something you typically do when the diagram source code is specified inline and you want to have control over the name of the generated diagram. |
My guess is that an earlier run resulted in a file being placed into the cache with the contents of a different diagram. @Kira-Cesonia, are you aware of how to discard the cache? Does it still happen then? I agree that the plantuml block macro is more suited for this task. |
Interesting, however, I note that while this solution prevents the wrong diagram to be included on all occasions...:
...this one, while working, still includes the wrong diagram at times:
The reason for this is an import in another file that was phrased as this:
So, what happened? Everything summed up again:
In another file, somewhere else in the project, coincidentally, the same header is used, but for the "correct" file.
What happens now?
However, writing it like this causes the intended behaviour of
|
I'll have a think about how we can improve this, but fundamentally what you're asking the system to do is very poorly defined. When you write
that Since the system is writing to the same destination file in both cases, you're going to end up having one of the two images in the final results; last processed diagram wins. I don't think it would be a good idea to second guess the user of the system here and start generating different filenames, but I think it should be feasible to generate a warning. |
I like the idea of generating a warning if the same filename is used twice in the same conversion. The only case that would be valid is if the same diagram is being shown in two different places in the document...but even then I would recommend two different filenames. |
I think the point would be to throw a warning, or maybe even an error if a directive would cause the system to try and create a file with a filename that already exists. Any other system I can think of will either throw an error or a warning in that case. If you try to create a file with a name that already exists in pretty much any operating system, you will get a warning and a question of whether you want to overwrite the file. Meanwhile, most programming languages will not even allow you to declare the same variable twice. If you declare a variable somewhere, and then declare a variable of the same name within the same scope, you will get a compiler error. Considering that Asciidoc is closer to programming than to GUI experiences, I would really suggest making this a case for a compiler error following the above logic. The error message should be something along the lines of:
However, something to consider here is that at least in the project that I have inherited, the following structure was consistently used throughout the code:
That is, we have multiple such "declaration blocks". It may not be the "as intended" way to do it, but I think it is a valid use case since it does work. Naturally, we don't want to throw errors or even warnings if a documentation uses the same such block in multiple places as long as the header and the contents of the block are identical. Summed up, it is probably going to be tricky finding a perfect solution for this, but the bottom line is that we need a feedback mechanism that notifies users of why the wrong diagram is displayed if such a case happens. The current system is not very intuitive in that way. Reasoning why it is not intuitive:
or
...then it is intuitive that the diagram that should be displayed at this location should be the diagram specified by filename. |
Asciidoctor doesn't work that way. We log messages, then you can decide which level of message should produce a program failure. The reason for this is that many users of Asciidoctor are not programmers and an aborted program is unfriendly to that audience and is perceived as a crash (not to mention disruptive to writing). We try to be informative to help the writer course correct in the next round of edits. Asciidoctor is not a compiler. It's a writing tool. |
To you. But as Pepijn points out, what you've done is actually valid operation. It just isn't desirable. The program cannot read your mind. But as we've discussed, it's reasonable to add an assumption that no two diagram blocks should have the same name (which is a new assumption), which would help avoid this type of user error and thus be more friendly. |
Okay, let me ask it like this: |
You're looking the problem only through the lens of your situation. How is Asciidoctor Diagram going to make any sense of a name mixup? You named one diagram something in English, another diagram something in English, and to you, the names are wrong. To Asciidoctor Diagram, it sees a string of characters. For all it knows, that's what you meant. You are the one that mixed up the names, not Asciidoctor Diagram. (To help clarify, Asciidoctor Diagram never sees the name of the include file. That information is simply not visible to the extension. It only sees the filename if you use the block macro). Having said that, we've already established that if the same diagram name is appearing twice in the same document (which Asciidoctor Diagram could confirm is or is not the same diagram content), that it's an indication something is wrong and a warning would help the user find the mistake and fix it. |
@pepijnve - As a proof of concept I've added a monkey-patch based on 1.5.18 version of the asciidoctor-diagram that is currently used in the IntelliJ plugin. My ruby skills are limited, feel free to criticize, improve or discard it. I'm looking forward to throw away this piece of code as soon as possible 🙄 The method check_duplicate_target() adds a warning to the two conflicting diagrams with the same name referencing each other with file and line number. It uses a dummy attribute per target file name in the document as "memory" (as a quick-and-dirty solution to keep something with the scope of the document). |
@ahus1 thanks for getting the ball rolling on this one. @mojavelinux any suggestions as to what the most appropriate way to do this is? |
Is asciidoctor-diagram the only extension that generates images during conversion process? Can't it be that there is a collision between multiple extensions? |
Good question, how can I check that? |
I was asked to post the following issue in this project instead of asciidoctor-intellij-plugin:
The issue is highly counter-intuitive logic when including diagrams such as from PlantUML in the IntelliJ plugin. I was told by a contributor that this is caused by the underlying Asciidoctor Diagram logic
I am using the following statement to include a Plant UML diagram on a page:
However, instead of including ../diagrams/containers.puml as intended, this includes a different diagram from the seemingly unrelated resource of ../external-references/diagrams/external-components.puml, and then scales it up to the size of ../diagrams/containers.puml`.
For some reason, this only happens for this one includes statement. The following inclusion of a different PUML diagram, for example, works no matter where in the AsciDoc I put it:
Now, as mentioned before, I managed to get it to work by changing the whole statement to the following (removing ",containers" in the header):
By now I understand that the second parameter in the block attributes sets a global filename that is stored in "Mystic Space" and can cause issues even in smaller projects with only a few dozen documentation pages and diagrams (this happened within a week of us setting up our first documentation). What is the benefit of this parameter anyway, since you still need to pass the exact path and filename in the include statement afterwards? According to my experimentation, omitting it does not cause any issues. I intuitively expected block to work on its own without needing an extra import statement below if you've declared and named it once, but alas, that does not seem to be the case.
Aka, this works:
This also works:
However, I intuitively expected this to work, but it does not (expected it to display the same diagram twice, but it only displays it once):
Instead, this is required to display the same diagram twice:
But that could also be shortened to:
So, what is the advantage of including the filename in the block? As I see it, it only serves as a source of Type 3 refactoring errors.
Quote ahus1:
Also see the original issue in the asciidoctor-intellij-plugin issue tracker: asciidoctor/asciidoctor-intellij-plugin#388
The text was updated successfully, but these errors were encountered: