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

Display reaction names in diagrams and deprecated the @label attribute #2016

Closed
wants to merge 2 commits into from

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Sep 21, 2023

Fixes #1856. Note that this does not ensure yet, that the target code generators actually name the reactions as given in the LF code. For targets not supporting bodyless reactions, this is purely cosmetic and not a strict requirement.

@cmnrd cmnrd added the enhancement Enhancement of existing feature label Sep 21, 2023
@cmnrd cmnrd requested review from lhstrh and a-sr September 21, 2023 14:31
@lhstrh
Copy link
Member

lhstrh commented Sep 21, 2023

I haven't looked at the code, but let's make sure to keep the @label annotations for reactors, because I think they are useful for supplying additional information.

import org.lflang.lf.Model;
import org.lflang.lf.Reactor;
import org.lflang.lf.StateVar;
import org.lflang.lf.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style guide forbids these:

Wildcard imports, static or otherwise, are not used.

commentText = ((Reaction) element).getName();
}
// If the element does not have a name, check if it has an @label attribute
// TODO: the @label attribute is depricaetd and should be removed at release 0.7.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO: the @label attribute is depricaetd and should be removed at release 0.7.0
// TODO: the @label attribute is deprecated and should be removed at release 0.7.0

if (element instanceof Reaction) {
commentText = ((Reaction) element).getName();
}
// If the element does not have a name, check if it has an @label attribute
Copy link
Member

@lhstrh lhstrh Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still be able to write @label("Some description") next to a reactor class and show it. I think that this change should cement that for reactions, either the reaction name (if it is specified) or a numeral is shown inside the chevron, but I'm starting to think that we should still keep the @label around (also for reactions) to serve as additional explanation in the diagram, just as we have them for reactor classes.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 22, 2023

I think we should still be able to write @Label("Some description") next to a reactor class and show it. I think that this change should cement that for reactions, either the reaction name (if it is specified) or a numeral is shown inside the chevron, but I'm starting to think that we should still keep the @Label around (also for reactions) to serve as additional explanation in the diagram, just as we have them for reactor classes.

Yes, I actually had the same thought. Otherwise, we would be conflating names with annotations. I'll try to find an alternative solution.

@cmnrd cmnrd marked this pull request as draft September 22, 2023 15:16
@cmnrd cmnrd marked this pull request as ready for review September 27, 2023 06:42
@cmnrd cmnrd marked this pull request as draft September 27, 2023 06:42
@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 27, 2023

Closing this in favor of #2030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show reaction names in diagrams
2 participants