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

Please expose option to format JavaDoc #724

Open
ChristianCiach opened this issue Apr 26, 2022 · 9 comments
Open

Please expose option to format JavaDoc #724

ChristianCiach opened this issue Apr 26, 2022 · 9 comments

Comments

@ChristianCiach
Copy link

ChristianCiach commented Apr 26, 2022

In #152 you've disabled the formatting of JavaDoc by hardcoding a "false" value.

Google-Java-Format provides a command-line switch (and API) to enable/disable the formatting of JavaDoc. I would like you to do the same :)

@carterkozak
Copy link
Contributor

This sounds reasonable to me, I've been meaning to add support for this feature to support our javapoet formatter, Goethe.

@ChristianCiach
Copy link
Author

ChristianCiach commented Apr 26, 2022

That being said, I am not sure about the API while keeping backwards compatibility.

In my eclipse-plugin I am currently using the ServiceLoader to retrieve an instance of com.palantir.javaformat.java.FormatterService, which is an interface with these two methods:

    ImmutableList<Replacement> getFormatReplacements(String input, Collection<Range<Integer>> ranges)
            throws FormatterException;

    String formatSourceReflowStringsAndFixImports(String input) throws FormatterException;

For keeping backwards compatibility maybe we should change these two methods to "default" methods to delegate to new methods that take an additional parameter boolean formatJavaDoc, like so:

    ImmutableList<Replacement> getFormatReplacements(String input, Collection<Range<Integer>> ranges, boolean formatJavaDoc);

    String formatSourceReflowStringsAndFixImports(String input, boolean formatJavaDoc) throws FormatterException;

    default ImmutableList<Replacement> getFormatReplacements(String input, Collection<Range<Integer>> ranges)
            throws FormatterException {
        return getFormatReplacements(input, ranges, false);
    }

    default String formatSourceReflowStringsAndFixImports(String input) throws FormatterException {
        return formatSourceReflowStringsAndFixImports(input, false);
    }

This way we could keep the old methods (maybe deprecating them).

@q3769
Copy link

q3769 commented Nov 21, 2023

#944

@ChristianCiach
Copy link
Author

Thanks for notifying me. Closing this issue as done.

@q3769-patientpoint
Copy link

q3769-patientpoint commented Nov 23, 2023

Thanks for notifying me. Closing this issue as done.

Sorry why is this one closed? It's not done.

There are couple of related issues: One is for Geothe, which is done; the other is for IntelliJ/idea plugin, which will most likely be automatically done if this current issue is worked on and completed.

My understanding of the current issue/request is to provide an option to enable Javadoc formatting on the CLI or core level, which is not done.

@ChristianCiach
Copy link
Author

Oops, I didn't check. I thought this option was also exposed with the Idea plugin.

@blutorange
Copy link

One question just to make I did not misread this: the option to format JavaDocs should disabled by default, but we can add an option to other tools that make use of Palantir, e.g. diffplug/spotless#1995 ?

@q3769-patientpoint
Copy link

q3769-patientpoint commented Jan 13, 2024

@blutorange: The request(s) by the original requester(s) was to add option to enable JavaDoc formatting; so yes, in that sense the default is "Javadoc formatting disabled".

However, if we are still accepting more votes/opinions on what should be the "reasonable default", then MHO is that the default should be "format-javadoc=true" and the (customization/configurable) option should be "format-javadoc=false".

Many thanks.

blutorange pushed a commit to blutorange/palantir-java-format that referenced this issue Jan 21, 2024
* This adds an option to the IDEA plugin (checkbox) to format JavaDoc comments.
  It is disabled by default.
* As the IDEA plugin (sometimes) uses the bootstrapping formatter service, which in turn
  uses the command line tool, this also add an option `--format-javadoc` to
  the command line tool.
* To pass the options to the FormatterService loaded via service provider, I added a
  method `withOptions` to the service provided interface, which returns a new (immutable)
  instance with the updated options. Compared with the suggestion from the issue comment
  (palantir#724 (comment)),
  this (a) does not require deprecating methods and (b) the options can be set independently
  of where the formatter is actually used.
* Since the infrastructure to pass options to the formatter now exists, we could also
  add more options to the code style select configuration in the IDEA plugin, but I'm
  not sure if that makes sense (I want to use Palantir, after all).
* The issue also mentioned API compatibility. The service provider interface is compatible,
  most other classes and methods are internal. But if somebody more familar with the code
  base could take another look, that would be great.
* I also added a few tests for the new option.
@blutorange
Copy link

Thanks for the info. For the time being I made a PR to add an option to the IDEA plugin (and CLI).

Personally, I'm not quite so fond of the way it formats doc comments, such as potentially long indents in @param or its liberal usage of self-closing tags. If I had to choose, I like the Eclipse formatter better for doc comments. In that regard, I can understand why some people would rather not have it be turned on by default. Still, it's not that bad and the formatted comments look mostly ok to me. Also, let's not forget, it's a great improvement over not formatting comments at all. Having to insert line breaks manually is quite cumbersome when you work a lot with doc comments.

Since we're probably going to enable JavaDoc formatting, we would not mind if it were turned on by default.

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

No branches or pull requests

5 participants