archimatetool / archi

Archi: ArchiMate Modelling Tool
https://www.archimatetool.com
MIT License
927 stars 267 forks source link

How to include colon (:) inside an if label expression? #794

Closed heikkil closed 2 years ago

heikkil commented 2 years ago

Version of Archi

4.9.1.

Archi Plug-ins

Operating System

Expected Behaviour

A label expression should be able to contain a colon (:) character in the output. The if statement uses colon as an internal separator. I can not find a way to escape the colon. The only online source of information for label expressions seems to be their wiki page. Is there more somewhere else, e.g. a formal language description?

Actual Behaviour

This works with semicolon, if property 'en' has a value 'value': ${if:${property:en}:en;${property:en}} --> en;value

With colon: ${if:${property:en}:en:${property:en}} --> en

Phillipus commented 2 years ago

If you'd like to have a go at fixing it:

https://github.com/archimatetool/archi/blob/master/com.archimatetool.editor/src/com/archimatetool/editor/ui/textrender/IfRenderer.java

jbsarrodie commented 2 years ago

I can easily fix this to accept escaping with a backslash. I'll have a look.

jbsarrodie commented 2 years ago

I can easily fix this to accept escaping with a backslash. I'll have a look.

@Phillipus I've just pushed a fix. It allows escaped colons in the IF, THEN and ELSE clauses. Because \: is now replaced by :, I've also added a way to escape backslash so that it is still possible to write \: using \\\:

Phillipus commented 2 years ago

@jbsarrodie Great! I'll add some unit tests. Can you think of some examples to test with?

jbsarrodie commented 2 years ago

@jbsarrodie Great! I'll add some unit tests. Can you think of some examples to test with?

${if:not\:Empty:Is\:OK:Not\:Shown} should render as Is:OK ${if::Not\:Shown:Is\:KO} should render as Is:KO

Phillipus commented 2 years ago

Unit tests added. Thanks!

Phillipus commented 2 years ago

The fix is present in Archi 4.9.2 beta 3. Please test. đź‘Ť

https://www.archimatetool.com/beta/

heikkil commented 2 years ago

I downloaded the beta and tested. Escaping single characters works when there are no variables in the statement. Looks like my original example was not tested and escaping and pattern matching is not yet working correctly:

The behaviour with an escaped colon is: ${if:${property:en}:en\:${property:en}} --> en\

If the escaped colon is not in front of a dollar variable, it works as expected. Please check your regular expressions matches in this specific case, too.

Phillipus commented 2 years ago

The backslash before the colon is not showing. Should be:

${if:${property:en}:en\:${property:en}}
jbsarrodie commented 2 years ago

The backslash before the colon is not showing. Should be:

This doesn't work either. The issue is that the colon after the backslash is interpreted as the separator of an IF:THEN:ELSE expression. This is a bit more complex but can be solved by making sur the colon used as separator is not just after a backslash.

I'm on it :-)

jbsarrodie commented 2 years ago

In fact, no need to even have nested expression, the issue is also in: ${if:ok:en\:ok} which renders as en\ instead of en:ok

jbsarrodie commented 2 years ago

I'm on it :-)

FIxed in latest commit

Phillipus commented 2 years ago

Thanks, JB. I'll do another beta build.

Phillipus commented 2 years ago

Please try Archi 4.9.2 beta 4.

https://www.archimatetool.com/beta/

heikkil commented 2 years ago

Now it works. Thanks!

I tried to stress test this a bit more and it does work as it should. However, look at this:

${if:ok:en\:ok}       --> en:ok
${if: ok : en\ : ok}  --> en : ok
${if : ok : en\ : ok} --> x

'if:' works but 'if :' is not recognised. This might be the way you intended, but it leaves a bit uncomfortable feeling that colon is used in more than one role here: The real function to call is not 'if' but 'if:'. Having a powerful syntax for label expressions is really needed, but I think you should be careful and think ahead how you want to extend it so that you do not work yourselves in a dead end. The first step in language formality is to put together a Backus–Naur form for it. I'd love to see that.

Phillipus commented 2 years ago

Thanks for the feedback.

@jbsarrodie is the space something to be concerned about?

Note - I would rather keep label expressions as simple as possible. There's already a bit of an overhead when parsing the regex for objects. Also, regex is not my strength, so if issues arise I might not be able to fix them.

Phillipus commented 2 years ago

is the space something to be concerned about?

As I'm keen to release Archi 4,9.2 very soon perhaps this is something that could be looked at next time.

heikkil commented 2 years ago

For my part, I do not see this as a showstopper. I'd go for a release.

jbsarrodie commented 2 years ago

'if:' works but 'if :' is not recognised. This might be the way you intended, but it leaves a bit uncomfortable feeling that colon is used in more than one role here: The real function to call is not 'if' but 'if:'. Having a powerful syntax for label expressions is really needed, but I think you should be careful and think ahead how you want to extend it so that you do not work yourselves in a dead end. The first step in language formality is to put together a Backus–Naur form for it. I'd love to see that.

I don't consider that label expressions are a real language, and we don't really parse them, we isolate some patterns and substitute with other values. We also decided to be backward compatible with expressions defined by Hervé in his great specialization plugin.

I don't think colon is used in more than one role: it is used as the separator between operators and values. I did think about allowing optional space and newlines, but this would come with lots of ambiguity and constraints for users as it would them be needed to escape leading and trailing spaces if they are wanted. In addition, as Phil said, we also want these expression to as lightweight as possible to not add unwanted overhead.

@jbsarrodie is the space something to be concerned about?

Not in the short term.

As I'm keen to release Archi 4,9.2 very soon perhaps this is something that could be looked at next time.

Yes, IMHO this might be looked at, but in the future, not for 4.9.2

Phillipus commented 2 years ago

Branch has been merged to master.