Feature/folding#22
Conversation
This class already did that, but without telling java
wouterjsmit
left a comment
wouterjsmit
left a comment
There was a problem hiding this comment.
Overall seems fine, some things must be better documented and the common ancestor of the Fold and PrettyPrint functions/results could use a thought.
| /** | ||
| * Helper to wrap {@link IStrategoTerm#writeAsString}. | ||
| */ | ||
| private static class Helper implements Appendable { |
There was a problem hiding this comment.
Documentation regarding why this class exists and what it does (it breaks lines on parentheses) is not clear enough. Possibly rename to better reflect what it does in the class name?
| * | ||
| * @return {@link PrettyPrintFunction} - The pretty print function. | ||
| */ | ||
| public FailableFunction<ISpoofaxTermResult<?>, PrintResult, IResult> pPrettyPrintFunction() { |
There was a problem hiding this comment.
I wouldn't go with the p prefix. It is used by other functions to denote whether they were chained to analyze or parse, but in this case it is evident.
Possibly even consider prefixing with term to denote that it requires an ISpoofaxTermResult<?>
| /** | ||
| * Helper to wrap {@link IStrategoTerm#writeAsString}. | ||
| */ | ||
| private static class Helper implements Appendable { |
There was a problem hiding this comment.
Just as with the Helper in the FoldFunction, this could be better explained.
| * The wrapped {@link ISpoofaxParseUnit}. | ||
| * @param regions | ||
| * The {@link IRegionStyle}s in terms of {@link IStrategoTerm}s. | ||
| */ |
| /** | ||
| * Represents a result that is a term and foldable regions. | ||
| */ | ||
| public class FoldResult implements IResult { |
There was a problem hiding this comment.
Should it not maybe inherit from an ISpoofaxTermResult, seeing that this result also has a term?
| /** | ||
| * A result that represents output that is human readable. | ||
| */ | ||
| public class PrintResult implements IResult { |
There was a problem hiding this comment.
Since this class seems to depend on the Fold regions, would it not be wise to make a common ancestor/somehow denote this? They both have a List<ISourceRegion> which is meant to be the same fold regions, right?
There was a problem hiding this comment.
Result would then be a common ancestor with empty children to let Guice differentiate, or do the binding more explicit. Treating them as separate yields a clearer structure at the cost of minimal (seemingly) redundancy.
| * @return {@link FailOrSuccessResult} - A {@link PrintResult} containing a human readable | ||
| * text plus fold regions, or a failed result. | ||
| */ | ||
| FailOrSuccessResult<PrintResult, IResult> print(ISpoofaxTermResult<?> input); |
There was a problem hiding this comment.
This also provides folding right? Maybe foldAndPrint or simply fold is more appropriate (result is a textual (pretty-printed) representation of a folding).
There was a problem hiding this comment.
The idea is that a printresult is (potentially) foldable and the print logic understands the foldregions. Calling this function merely asks to print a given input, thats why its just called print. I can't think of cases in which this method will be used without the result containing foldRegions so I renaming it is ok by me.
justinvdk
force-pushed
the
feature/folding
branch
3 times, most recently
from
e8cddc2 to
300b461
Compare
Ugly workaround but it works for now
justinvdk
force-pushed
the
feature/folding
branch
from
300b461 to
c3cf8ea
Compare
2 participants
Implements folding for term results (like parsing).