Automating Formatting of Rune Code #859
Conversation
|
| import org.eclipse.emf.ecore.resource.Resource; | ||
|
|
||
| public interface CodeFormatterService { | ||
| List<Resource> formatCollection(List<Resource> resources) throws IOException; |
There was a problem hiding this comment.
If this method modifies the resources in-place, I would change this to void. A bit of documentation to explain this behaviour would also be good.
There was a problem hiding this comment.
I think the type of the input can be loosened to Collection.
|
|
||
| import org.eclipse.emf.ecore.resource.Resource; | ||
|
|
||
| public interface CodeFormatterService { |
There was a problem hiding this comment.
This name is perhaps a bit generic - it doesn't really tell you what it does. What about ResourceFormatterService?
| Resource formattedDocument = formatDocument(doc); | ||
| result.add(formattedDocument); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
This is perfectly fine, but just as a challenge, could you rewrite this using Java stream API? :)
| private Provider<FormatterRequest> formatterRequestProvider; | ||
|
|
||
| @Inject | ||
| private Provider<RosettaFormatter> rosettaFormatterProvider; |
There was a problem hiding this comment.
This one is injecting the implementation rather than the interface. Change to inject the formatter interface instead.
Just to explain why: this is against the principle of dependency injection, since it breaks the separation between interface and implementation, making the codebase less extensible.
| return result; | ||
| } | ||
|
|
||
| private Resource formatDocument(Resource doc) throws IOException { |
There was a problem hiding this comment.
Since the word Document is used for something specific in Xtext's formatting code (e.g., IFormattableDocument), the name formatDocument might be misleading. What about formatResource?
There was a problem hiding this comment.
I wonder if we should expose this method in the interface as well. It might be good if at some point a use case arises for formatting a single resource.
| import javax.inject.Inject; | ||
| import javax.inject.Provider; | ||
|
|
||
| public class RuneFormatter implements CodeFormatterService{ |
There was a problem hiding this comment.
Make name of class consistent with interface.
| INode node = NodeModelUtils.getNode(content); | ||
|
|
||
| TextRegion textRegion = new TextRegion(node.getOffset(), node.getLength()); | ||
| req.setRegions(Collections.singletonList(textRegion)); |
There was a problem hiding this comment.
Have a look at the documentation of regions inside FormatterRequest. I think this can be simplified.
| TextRegion textRegion = new TextRegion(node.getOffset(), node.getLength()); | ||
| req.setRegions(Collections.singletonList(textRegion)); | ||
|
|
||
| ITextRegionAccess regionAccess = regionBuilder.forNodeModel((XtextResource) doc).create(); |
There was a problem hiding this comment.
This is interesting. I don't think we should cast here.
Just some background: in our case, all resources are XtextResources for now. However, there is support for adding other kinds of resources as well to a resource set. Suppose for example that we somehow would add support for loading .csv files or .xml files or other "raw" resources, then this will crash.
Instead what I think we should do is change the input type to XtextResource (because other resources are not supported), and then in the method formatCollection filter out any non-XtextResources.
|
|
||
|
|
||
| //formatting using TextRegionRewriter | ||
| TextRegionRewriter regionRewriter = new TextRegionRewriter(regionAccess); |
There was a problem hiding this comment.
See docs on class for the recommended way of getting an instance. :)
|
|
||
| private Resource formatDocument(Resource doc) throws IOException { | ||
| //setup request and formatter | ||
| FormatterRequest req = formatterRequestProvider.get(); |
There was a problem hiding this comment.
I wonder if we should add some configuration to this request as well. (perhaps even a way to pass in ITypedPreferenceValues to configure the formatting?)
|
|
||
| @ExtendWith(InjectionExtension.class) | ||
| @InjectWith(RosettaInjectorProvider.class) | ||
| public class FormatterServiceTest { |
There was a problem hiding this comment.
Should have a consistent name, e.g., <ServiceInterfaceName>Test
| @Inject ISerializer serializer; | ||
|
|
||
| @Test | ||
| void formatSingleDocument() throws IOException, URISyntaxException { |
There was a problem hiding this comment.
Since formatSingleDocument and formatMultipleDocuments share a lot of code, I think it would improve maintainability if we added a little test utility method to this class. E.g., you pass it a list of files, and the test will format those files and compare them to their expectation.
| @Override | ||
| public void formatCollection(Collection<Resource> resources) { | ||
| formatCollection(resources, null); | ||
| } |
There was a problem hiding this comment.
I prefer trivial implementations such as these as a default interface method instead, so implementers of the interface do not have to write the same boilerplate code. I would move this one up, together with the two below.
| e.printStackTrace(); | ||
| } | ||
| } else { | ||
| System.out.println("Resource is not of type XtextResource and will be skipped: " + resource); |
There was a problem hiding this comment.
Let me introduce you to the wonderful world of logging APIs :)
| } | ||
|
|
||
| @Override | ||
| public void formatXtextResource(XtextResource resource, ITypedPreferenceValues preferenceValues) throws IOException { |
There was a problem hiding this comment.
I'm surprised to see a throws IOException in this declaration, since conceptually this method does not perform any I/O. More below.
| //With the formatted text, update the resource | ||
| InputStream resultStream = new ByteArrayInputStream(formattedString.getBytes(StandardCharsets.UTF_8)); | ||
| resource.unload(); | ||
| resource.load(resultStream, null); |
There was a problem hiding this comment.
I'm guessing you added the IOException to the declaration because this method potentially throws an IOException. This is because in general the input stream could come from anywhere, e.g., from reading a file, or from a HTTP request over the internet. However, we know that the input stream is actually coming from an in-memory string (so no I/O happening), so in practice I would not expect an IOException here, so I don't think we should leak it to the callers of this method.
That's a long way of saying I think we should catch the error here, wrap it in an UncheckedIOException, and then rethrow it. :) (together with a small comment explaining why we expect an IOException never to happen)
This PR deals with issue #858.
Type of change