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
review: refactor: PrinterHelper to CommentHelper, LiteralHelper and OperatorHelper #1525
Conversation
/** | ||
* Incrementation post assignment. | ||
*/ | ||
POSTINC, // _ ++ | ||
POSTINC(false), // _ ++ |
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.
I'd say we already have the postfix information in the naming convention. If it starts with POST it's postfixed, else it's prefixed. Correct?
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.
Will this be OK from your point of view?
UnaryOperatorKind() {
this.prefixOperator = name().startsWith("POST")==false;
}
can the helper classes and methods be package visible? WDYT? |
ok, no problem. Just when somebody will LATER write own printer, these helper classes will be very useful. But till then they can be package protected, so we can minimize API. I will do so. |
This code could even be put in one of the helper methods, in order not to touch and impact this core
model interface.
|
I agree, it's important to be careful on the public API surface, so as to maximize our long-term API
stability.
If we create a new public class or method, then we have to document and test it extensively.
|
I guess you speak about Note: before my change it was not visible that there are two types of UnaryOperatorKind - prefix and postfix. With the method |
b1556e6
to
06d9017
Compare
I made helpers package protected. It needed move of I rolled back all the changes on operator enums. I created OperatorHelper methods public static boolean isPrefixOperator(UnaryOperatorKind o) {
return isSufixOperator(o) == false;
}
public static boolean isSufixOperator(UnaryOperatorKind o) {
return o.name().startsWith("POST");
} It is ready for merge from my point of view |
CI has same problems... |
Try to trigger it again: apparently there was some issue with Spoon website last days. |
CI finally built it. Please merge it or give me feedback, so I can continue with next steps ;-) |
Thanks Pavel! |
For the record: 1) because it impacts the core Spoon metamodel and this should be done super-carefully 2) it mixes the modeling responsibility and the pretty-printing responsibility |
First basic refactoring step needed to implement all the changes needed by #1494.