-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Include values for generics and stereotypes in the output #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for your pull request. I think you are on the right track with this! 👍
Hey @Enteee thanks for you review ! I pushed two commits to fix your suggestions. |
@paduc I reviewd it and pushed a new commit which fixes some minor formatting issues. By the way you can run There is still one open question from the review. I do now understand why you chose the design as-is, but I think it can maybe be improved. My main critique point is that the |
@Enteee sorry about the formatting. I did run the command but there might be some conflict with my IDE lint config... For the generics negative prefix, I don’t see how we can do without it but I’m no pegjs expert. I might learn something :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[removed, see next review]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paduc Added the following valid diagram to the tests:
@startuml
title Generics and Stereotypes
package Generics {
class TestOnlyGenerics <G1>
' class TestMultipleGenerics <G1> <G2> # Not supported
class TestGenericsWithAngleBrackets < <G1> >
class TestGenericsWithDoubleAngleBrackets < <<G1>> >
}
package Stereotypes {
class TestOnlyStereotype <<S1>>
class TestMultipleStereotypes <<S1>> <<S2>> <<S3>>
class TestStereotypeWithAngleBrackets << <S1> >>
class TestStereotypeWithDoubleAngleBrackets << <<S1>> >>
}
package mixed {
class TestGenericsAndStereotype <G1> <<S1>>
' class TestMultipleGenericsAndStereotype <G1> <G2> <<S1>> # Not supported
class TestGenericsWithAngleBracketsAndStereotype < <G1> > <<S1>>
class TestGenericsWithDoubleAngleBracketsAndStereotype < <<G1>> > <<S1>>
class TestGenericsAndMultipleStereotypes <G1> <<S1>> <<S2>>
class TestGenericsAndStereotypeWithAngleBrackets <G1> << <S1> >>
class TestGenericsAndStereotypeWithDoubleAngleBrackets <G1> << <<S1>> >>
}
@enduml
Key takeaways:
- Multiple generics are not supported -> Generics can stay a string
- Multiple stereotypes are supported -> stereotype should become an array
- Angle brackets seem to be partially supported in stereotypes and generics.
See this review for a more detailed list of things which we should change. I don't quite know how to solve all of them, but we should work on this before merging the pr.
@Enteee indeed I didn't know that generics and stereotypes could include angle brackets like that. I guess we can find a way to handle that in the parser. Should handling these special cases really block this PR though ? I'm thinking special cases could be handled in later iterations and another PR. There's one other case I think would be interesting to parse (in yet another PR) : |
Yes I agree, and I also do think its ok when we don't implement angle brackets handling for generics and stereotypes with this pull request. I will instead just open a bug report.
Honestly, I would not implement this. Especially not in this PR. If you do think there is value in doing this feel free to open an enhancement request. Where we then can discuss this. Nevertheless, there is still one thing that I would like to see changed in this PR: It is possible to declare multiple stereotypes:
Therefore the stereotype property should become a list of strings instead of just a string. And the grammar should be changed so that it allows multiple stereotypes. Do you think you can do this or should I have a look? |
opened the bug for angle brackets handling: #59 |
I agree ! I’ll have a look ASAP. |
@paduc how is it going with this? do you need any help? |
@paduc ok, no worries. Enjoy your holidays |
Fixes #42
In order to use the values inside the generics and stereotype syntax operators, I did the following:
generics_
andstereotype_
properties toClass
,Interface
andEnum
typesDecorators
intoStereotype
andGenerics
parsersStereotype
andGenerics
parsers return their valuesClass
,Enum
andInterface
parsers to make use of theStereotype
andGenerics
parsers and forward their values to the corresponding types.Please note that the allowed declaration format for classes, interfaces and enums changed a bit (without breaking any of the examples).
Previously, the parser would allow ex
class MyClass<generic> <<stereotype>> extends ... {
(the generic and stereotypes follow each other).After this PR, it allows ex
class MyClass<generic> extends ... <<stereotype>> {
(the generic is right after the class name and the stereotype is at the very end). This seemed better for me, as a stereotype is a sort of annotation. That is up for debate.