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

Reaction names should be used as a default #2355

Open
soerendomroes opened this issue Jul 8, 2024 · 14 comments
Open

Reaction names should be used as a default #2355

soerendomroes opened this issue Jul 8, 2024 · 14 comments
Labels
diagrams Problems with diagram synthesis

Comments

@soerendomroes
Copy link
Collaborator

I would like to use reaction labels as a default and not just their numbering.

Additionally, I would like the reaction label to be just the number if it does not have a label and not reaction_i for reaction i.
This way, the diagram would not change for models that do not have labeled reactions while still using a desirable default.

@soerendomroes soerendomroes added the diagrams Problems with diagram synthesis label Jul 8, 2024
@cmnrd
Copy link
Collaborator

cmnrd commented Jul 10, 2024

I am not sure if I follow. By reaction label, you mean the yellow box that appears when using the @label attribute on a reaction, right? Why should this become the default instead of showing the number within the reaction shape?

@soerendomroes
Copy link
Collaborator Author

soerendomroes commented Jul 10, 2024

I mean the label that is inside a reaction e.g. currently this is something like "1" or "2". One can declare a label for it, by doing something like "reaction 'insert name' (...)" which I think assigns more meaningful label to a reaction than "1". If the option show reaction labels is enabled, it currently shows the label if there is any but displays "reaction_1" for reaction 1 if it does not have a label. I think "1" should be a better default label and that reactions should be labeled as a good developer practice.

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 10, 2024

Ok, so it seems like you are referring to reaction names (not labels) and the "Reaction names" option. Right? This option is intended to show the name of the reaction as it is used in generated code (i.e. it represents the name of the generated function that implements the reaction body). Showing the name is particular important when bodyless reactions are used (i.e. we only code-generate a skeleton and the function implementation is provided externally).

It is important to distinguish label and name here. The label can be an arbitrary string. The reaction name needs to be a valid identifier, and in most languages numbers are not valid identifiers.

@soerendomroes soerendomroes changed the title Reaction labels should be used as a default Reaction names should be used as a default Jul 10, 2024
@edwardalee
Copy link
Collaborator

I'm not sure reactions should be named as good developer practice. The order of reactions is semantically important, so I think the diagram label should include the number even is the reaction is named. I agree that "reaction_1" is a pretty ugly label, and "1: reaction_1" would be even uglier...

@soerendomroes
Copy link
Collaborator Author

reaction_1 is the default name. I just want that the name is displayed as "1" instead of "reaction_1" if no name is specified.

Here the default "names" do nothing except waste space.
load_balancer_reaction-names

This should be the default names for reactions without a name.
load_balancer_reaction-numbers

@edwardalee
Copy link
Collaborator

Agreed. This should be a trivially easy fix...

@soerendomroes
Copy link
Collaborator Author

Additionally, my point is that labeling a reaction "send" is always better than just having a name that says "2".

@edwardalee
Copy link
Collaborator

Why? What does "send" mean? Not all reactions send anything.

@soerendomroes
Copy link
Collaborator Author

"send" would be the name I would want to give a reaction by writing reaction send (...) -> .... I would prefer this to leaving the reaction unnamed. It is just an example.

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 10, 2024

If we change this behavior, then the names displayed in the diagram do no longer correspond to their identifier in generated code. Displaying reaction_1 (the default name) is an important feature in my view, and losing the clear mapping between diagram and identifiers in code is a significant cost.

@soerendomroes
Copy link
Collaborator Author

@cmnrd I do not understand your statement. What are you referring to?

Why should "reaction_1" be better than "1"? Or if the reaction has a name, why should "1" or "reaction_1" be better than displaying "send" for a reaction I named "send" in the code? Even if the reaction code uses "reaction_1" and cannot use "1" as a name, I am pretty sure people can infer the "reaction_".
I do not see how we would lose anything by encouraging developers to name their reactions and by displaying "1" instead of "reaction_1" if we enable reaction names.

Additionally, we do never lose the connection between diagram and code since clicking on a reaction will highlight it in the code.

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 10, 2024

This option is intended to show the name of the reaction as it is used in generated code (i.e. it represents the name of the generated function that implements the reaction body). Showing the name is particular important when bodyless reactions are used (i.e. we only code-generate a skeleton and the function implementation is provided externally).

To explain this further: We introduced reaction names to provide bodyless reactions. For bodyless reactions, the reaction body is provided externally (not in the LF file). The user needs to implement a function that is appropriately named. For reaction send (...) the user needs to implement a function called send in the target language. For an unnamed reaction (reaction (...)) the user needs to implement a function called reaction_N in the target language where N refers to the reaction's priority. The purpose of the "Reaction names" option when it was introduced was to support users of bodyless reactions. It establishes a mapping between the diagram and the external definitions of reaction bodies in target code files. This mapping breaks if we only display "1". Furthermore, the reaction name is semantically an identifier, and integer numbers are not a valid identifier.

Since initially you called the reaction names labels, it seems like you have a different use-case in mind. Could you elaborate on your use-case and why the @label attribute is not sufficient?

@soerendomroes
Copy link
Collaborator Author

I called them labels since they are KLabels (I think). Name or label is just an overloaded term.
I am unfamiliar with the difference of @label and just naming reactions. As I understand from you is that the label is just a string that a reaction is named and name is the function in the code.
That I am missing is why labels and names are not the same, since they seem redundant.

Generally, I think that reaction send ... is much more explicit a better syntax than `@label("send") reaction ....

I do not want that "1" can be a name. I just what that the "label" of the KNode in the diagram that represents the reaction if names should be displayed in a readable and concise manner.

But if you do not like it, feel free to not implement this and close the ticket.

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 10, 2024

But if you do not like it, feel free to not implement this and close the ticket.

Just to be clear: I did not comment on the quality of the proposal or how I like it. I actually think your user story makes sense. I am pointing out that there is a user story that we already have committed to that conflicts with the concrete implementation that you propose. That is not a matter of my personal taste.

As I stated before, I think your user story makes sense, and I don't think it is necessarily at odds with the existing one. I am sure there is an alternative implementation that allows us to achieve your goal without breaking an existing feature. I want to invite you to work on this constructively, and I am happy to assist in finding that solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagrams Problems with diagram synthesis
Projects
None yet
Development

No branches or pull requests

3 participants