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

Provide a way to use my own formatters #435

Open
Jurrie opened this issue Mar 3, 2021 · 10 comments
Open

Provide a way to use my own formatters #435

Jurrie opened this issue Mar 3, 2021 · 10 comments

Comments

@Jurrie
Copy link

Jurrie commented Mar 3, 2021

Is your feature request related to a problem? Please describe.
Some of my files are in a format for which I have written a formatter myself. I would like to use that formatter.

Also, this would give the option for other people to use different formatters, for example using a different formatter for JavaScript.

Describe the solution you'd like
I would like to add a dependency to the formatter configuration, just like "Basic Configuration Using External Resource". The artifact should contain a class that implements an interface.

Describe alternatives you've considered
I didn't - I'm a lazy person 😀

@ctubbsii
Copy link
Member

ctubbsii commented Mar 3, 2021

I'm not necessarily opposed, but why bolt on additional formatters to this plugin, rather than just create a separate plugin?

@Jurrie
Copy link
Author

Jurrie commented Mar 3, 2021

A separate plugin could also be implemented of course. But then I would need to implement some parts of this plugin myself, like caching formatted files so they do not get formatted over-and-over for no reason.

I suppose the idea popped up because I saw this plugin does more than format Java. It also formats CSS, HTML, JSON, JavaScript... Why stop there? Or maybe the other way around: why weren't those formatters implemented in separate plugins?

@ctubbsii
Copy link
Member

ctubbsii commented Mar 3, 2021

why weren't those formatters implemented in separate plugins?

I think they should have been. I think this plugin is a bit too bloated and not as lightweight as it should be. There have been previous similar requests to extend the formatting functionality (see previous issues) and an open issue to at least move the existing ones into separate executions, if not separate plugins (#254) which would help address certain constraints on how to match up files to specific formatter executions (#427).

I'm not a huge fan of adding a bunch of random unrelated formatters for different languages into a single monolithic plugin. However, I kind of like your suggestion to make it extensible. I think I'd be in favor of that, if the configuration were easy to use, and it was done as a separate mojo.

In any case, if this were to be implemented, it would be a big change.

@Jurrie
Copy link
Author

Jurrie commented Mar 4, 2021

I think the best way would be implementing a service loader, but I agree it would be a big change.

Also, some thought needs to be given to how the configuration would look like. For example: should you configure the directories to format at a global level, or per formatter? What would be your take on this?

@ctubbsii
Copy link
Member

ctubbsii commented Mar 4, 2021

Per-formatter, for sure. I was thinking something like:

<plugin>
  <groupId>net.revelc.core.formatter</groupId>
  <artifactId>formatter-maven-plugin</artifactId>
  <version>${formatter.version}</version>
  <dependencies>
    <dependency>
      <groupId>com.example</groupId>
      <artifactId>external-formatter</artifactId>
    </dependency>
  </dependencies>
  <executions>
    <execution>
      <id>some-external-execution</id>
      <goals>
        <goal>external</goal>
      </goals>
      <configuration>
        <formatterId>MyExternalFormatter</formatterId>
        <includes>
          <include>**/*.ext</include>
        </includes>
        <excludes>
          <exclude>src/main/resources/alreadyformatted.ext</exclude>
        </excludes>
        <formatterOptions>
          <!-- formatter-specific options -->
          <configFile>value</configFile>
        </formatterOptions>
      </configuration>
    </execution>
  </executions>
</plugin>

The formatterId could either be a fully-qualified class name, simple class name of an interface-implementing formatter class discovered by the ServiceLoader from the class path, or a unique match on a String returned by a new method added to the interface (like String getId();) from an interface-implementing formatter class discovered by the ServiceLoader from the class path.

@Jurrie
Copy link
Author

Jurrie commented Mar 4, 2021

That would work, but would this require me to configure things like file encoding and line endings on a per formatter basis? Those probably do not differ between formats, I would think? Wouldn't it be nice to be able to configure this on a global level?

Maybe a global <formatterOptions> section could be supported? (Maybe that should be key+value pairs in that section, not a path to a config file.) Stuff that's configured in the global section would hold for all formatters, and the <formatterOptions> section per formatter would override. Granted: there is nothing in place that guarantees that keys in the global section are used in all formatters. For example: the Java formatter could listen to a key named "lineEndings", while the XML formatter uses "line_endings". Or values like "CRLF" vs "CR+LF". So this method wouldn't be bulletproof.

Another way would be to identify all stuff that you would want to configure globally, and have the interface define methods for that, like setLineEndings and setFileEncoding. When using an abstract class as the contract for the service loader, you could even extend this in later releases. (Or, using Java 8, you could have an interface with default methods.)

@ctubbsii
Copy link
Member

ctubbsii commented Mar 4, 2021

My suggestion to have a formatterOptions would be for containing formatter-specific config. The idea was arbitrary key-values, but in my example, the external formatter just had one known key-value, for an external config file.

Global options could still be used as-is, for line-endings, encoding, etc. I just didn't include them as part of my example. You would place such things directly under the configuration of the plugin, rather than in a specific plugin execution's config, and they would be merged.

Regarding line endings specifically, a way they can be consistent across formatters is to make the formatter interface be a mapping of List<String> inputLines -> List<String> outputLines, and the global code should be left to handle encoding and line endings as it writes the outputLines to the file. Of course, this might be redundant for some formatter implementations, which already have their own line ending feature.

@ctubbsii
Copy link
Member

ctubbsii commented Mar 4, 2021

We would also have to be careful about how the cache works. A separate cache per formatter makes the most sense, I think. So long as one formatter's cache hit doesn't prevent another formatter from doing its work.

@Jurrie
Copy link
Author

Jurrie commented Mar 5, 2021

My suggestion to have a formatterOptions would be for containing formatter-specific config. The idea was arbitrary key-values, but in my example, the external formatter just had one known key-value, for an external config file.

Global options could still be used as-is, for line-endings, encoding, etc. I just didn't include them as part of my example. You would place such things directly under the configuration of the plugin, rather than in a specific plugin execution's config, and they would be merged.

Looks good to me 😀

Regarding line endings specifically, a way they can be consistent across formatters is to make the formatter interface be a mapping of List inputLines -> List outputLines, and the global code should be left to handle encoding and line endings as it writes the outputLines to the file. Of course, this might be redundant for some formatter implementations, which already have their own line ending feature.

Hmm... I don't know enough about formatters to know if this would work or not. My gut feeling is that formatters expect a file rather than a List<String>. The latter case would also mean that the whole file is read into memory? Or will there be additional logic that streams the file to the formatter?

@ctubbsii
Copy link
Member

ctubbsii commented Mar 5, 2021

Most of the formatters, if not all, read the whole file into memory already. This should be fine. If you can't fit a file into memory, you need to learn to organize your code better. 😺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants