Common config-loader for Formatter using Properties. #89
Conversation
plugin-gradle/README.md
Outdated
| properties { | ||
| it.put('key', 'value') // specify other properties manually | ||
| it.put('key', 'value') // specify other properties manually | ||
| } |
There was a problem hiding this comment.
I prefer the old example. It's standard to have a gradle.properties file which contains project versions which are useful for markdown formatting. It's great to have the new shared parser, but I think the example should remain the typical usecase, rather than an exotic one.
The text can say something like "multiple properties files can be loaded, in any format supported by FormatterProperties." with a link to the FormatterProperties javadoc.
There was a problem hiding this comment.
👍
Yes. There will be a useful example of multiple files and formats when describing the groovy-formatter steps. This one is not useful.
| @@ -47,14 +40,14 @@ private EclipseFormatterStep() {} | |||
| private static final String FORMATTER_METHOD = "format"; | |||
|
|
|||
| /** Creates a formatter step for the given version and settings file. */ | |||
| public static FormatterStep create(File settingsFile, Provisioner provisioner) { | |||
| return create(defaultVersion(), settingsFile, provisioner); | |||
| public static FormatterStep create(Iterable<File> settingsFiles, Provisioner provisioner) { | |||
There was a problem hiding this comment.
This breaks binary compat, but it's very easy to have both methods.
public static FormatterStep create(File settingsFile, Provisioner provisioner) {
return create(Collections.singleton(settingsFile), provisioner)
}This way we keep binary compat between releases, and we don't have to modify as many tests.
There was a problem hiding this comment.
I principally agree, lib-extra is the supplier, plugin-gradle/plugin-maven are the users. Contract should not be broken.
But the basic idea is still to have a common interface and allow configuration injection by reusing files. So here should not be a user of the old interface, except the tests, which should by ignored for the interface design.
So the binary compat is a reason to keep the old interface and mark it as deprecated. But I would then still refactor the tests.
There was a problem hiding this comment.
I'm not opposed to supporting multiple config files.
But what is the reason to deprecate the specialized method for the common use-case of a single config file?
There was a problem hiding this comment.
The common interface, is actually imposed on the user interfaces. How plugin-maven will implement this interface is not known yet, but for plugin-gradle we once said that we stick with the existing methods and basically change File to File.... So newer versions of plugin-gradle
will not use FormatterStep create(File settingsFile, .... Currently this method has no user (I don't count tests), and I see currently no use case. Hence it is dead code and should be removed. If you want to keep it for binary compat, it should fade out, so it should be marked deprecated. @
There was a problem hiding this comment.
I disagree, but I don't think this issue is a big deal. Feel free to mark the legacy methods as deprecated.
In general, I'm wary of supporting theoretical usecases without a real example. Spotless is not flexible enough to allow different line endings on different lines of the same file - e.g. even lines \n, odd lines \r\n. If someone wanted to contribute this functionality, I think we would both reject it as too much complexity for too little practical value.
I don't understand the usecase of multiple property files for the eclipse formatter, but arguments are time consuming, your implementation is good and easy-to-read, so I happily give you the benefit of the doubt that this is a feature that at least you will use.
But as this change requires more churn to the existing code it becomes more expensive, and eats into its "benefit of the doubt" grant. The following are things that make a change more expensive, ranked from most to least:
- Change user-facing docs
- Break binary compat for a published lib
- Deprecate a method
- Write new code
I've made my point about the costs of this change as best I can. You understand the benefits of this change better than me, so I trust your judgement in weighing the benefits vs costs, and I'll accept this PR happily (deprecated or not) so long as it doesn't unnecessarily break binary compat.
There was a problem hiding this comment.
I would prefer the @deprecated because it transports the goal to the developer:
- Use the common user interface if your formatter is configured by properties.
- FormatterProperties is the central place to load the properties (from various file formats).
When I introduce my tests EclipseFormatterStepTest, I will (if you agree) anyhow refactor the EclipseFormatterStepTest to allign them with the once I have for GrEclipse. Why should I test the load of different property representations (formatter.properties/formatter.xml) in the formatter steps. That has been done already in this commit by the FormatterPropertiesTest.
Looking freshmark, I saw how you deal with property values which can be calculated at run time.
Maybe this can be once incorporated, but I do not know whether people would use that frequently. The code/resource formatting is in itself an encapsulated job. Big lists of configuration items should not mess up the build.gradle.
| <setting id="valid_xml_profiles.xml.null" value="Second profile should be skipped" /> | ||
| <setting id="common" value="Second profile should be skipped" /> | ||
| </profile> | ||
| </profiles> |
There was a problem hiding this comment.
Should we error out when a file contains two profiles? Users in the past have submitted bug reports because they didn't realize that their file had two profiles. Can we think of a context in Spotless when a user would have two profiles on purpose?
There was a problem hiding this comment.
Not exactly sure what you mean by "error out".
You refer to the log level?
Yes, maybe we WARNING is a better choice than QUIET.
There was a problem hiding this comment.
Sorry, just remembered that you stated explicitly in your documentation that only a single profile shall be exported. Hence the usage of multiple profiles was in the past considered erroneous, and therefore a warning appeared.
For Eclipse-JDT, the order can not be configured.
For the Formatter steps (and this is the only use case for this class), loading of multiple profiles is purposeless.
An extension of the configurability to select a profile can be considered unnecessarily complex.
So you are right, the usage of multiple profiles could lead to a failure.
But I should have never reduced the log-level to QUIET in the first place.
So I assume that you prefer a failure with a "informative" message how the user can fix the problem. I will implement the change correspondingly.
👍
There was a problem hiding this comment.
I mean throw either a RuntimeException or an Error. People ignore warnings.
What is the use case where a user will have a file with two profiles, and their intention is to silently ignore the second profile?
What is the value of supporting that use case, compared to the error mode that the user thinks they have saved their profile, but actually they have saved it only to the second profile?
My strong opinion is that we should fail hard when a file has multiple profiles. What is the use case where this behavior is against the user's wishes?
|
Funny, findbugs now complains. But only on test code, and on test code I have not changed. |
Configuration shall be reusable between formatter by sharing the same configuration file. Reused configuration shall also be modifiable by overwriting a sub-set of the reused properties. The FormatterProperties accepts zero to multiple files and currently supports Java properties, Eclipse profile and preference files. The FormatterProperties respects the configuration file order. In case the same property is specified in multiple files, new values override the old once.
|
Ok, Travis reported a different error. Don't know where the test errors on my local checkout came from. Vanished after clean and removal of gradle cache. Works now. Is there any change to see the findbugs results generated by Travis? |
|
FindBugs passed on Travis. Dunno what the failure is/was. This looks good to me. I'll merge at 8pm PST tonight unless I hear an objection. |
|
Looks okay to me, so no objections. :) |
|
The previous commit failed. Double null check. Fixed it by last commit. Please merge.
On 16/03/2017, 22:38 Ned Twigg <notifications@github.com> wrote:
FindBugs passed on Travis. Dunno what the failure is/was. This looks good to me. I'll merge at 8pm PST tonight unless I hear an objection.
—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Thanks for the help. Will provide change to formatter state tomorrow and groovy step this weekend...
On 16/03/2017, 22:47 Jonathan Bluett-Duncan <notifications@github.com> wrote:
Looks okay to me, so no objections. :)
—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
Configuration shall be reusable between formatter by sharing the same configuration file. Reused configuration shall also be modifiable by overwriting a sub-set of the reused properties.
The new FormatterProperties class accepts zero or multiple files; supports Java properties, Eclipse profile and preference files.
The FormatterProperties respects the configuration file order. In case the same property is specified in multiple files new values override the old once.