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

Quick up-to-date checking for custom rules (sub to #31) #47

Closed
nedtwigg opened this issue Oct 31, 2016 · 3 comments
Closed

Quick up-to-date checking for custom rules (sub to #31) #47

nedtwigg opened this issue Oct 31, 2016 · 3 comments

Comments

@nedtwigg
Copy link
Member

In order for incremental builds and up-to-date checking to work, we need FormatCheckTask to properly implement serializable, equals, and hashCode. This means that FormatStep and all of its implementations (eclipse, google-java-format, regex, etc.) will also have to implement these contracts, which is straightforward to do.

The downside is that custom rules, along the lines of:

custom 'No scanners', {
    if (it.contains('java.util.Scanner') {
        throw new AssertionError('please use BufferedReader instead')
    }
}

will always be out-of-date unless they properly implement the contract. One quick way to resolve this is to have a ForwardingFormatStep which delegates all of its serializable/equals/hashCode tasks to some delegate object. Then custom rules could easily be written as

custom 'No scanners', {
    if (it.contains('java.util.Scanner') {
        throw new AssertionError('please use BufferedReader instead')
    }
}, upToDateVersion=1

But if any rule in a list has changed, then the entire format has to rerun. Sooo, instead of checking each rule, we could instead check them globally across the format:

format 'someFormat' {
    custom 'someStep', {}
    custom 'someOtherStep', {}
    bumpThisNumberIfRulesChange(1)
}

If a project has even two custom rules, then this format-global approach is easier than having a version per custom rule.

The next question is whether we want to use this functionality to remove the requirement for FormatTasks to implement serializable/equals/hashCode at all, and just rely on bumpThisNumberIfRulesChange() to entirely encapsulate changes to the rules.

On the one side, we should have spotless be as fast as possible with as little user input as possible, and there are plenty of users that are only using built-in rules, so bumpThisNumberIfRulesChange() should be limited to a useful override for handling custom rules. Thoughts?

@oehme
Copy link

oehme commented Nov 1, 2016

I don't think we need this. Writing rules as standalone classes is straightforward enough for people who want the performance. Groovy has an EqualsAndHashcode annotation that does the boilerplate for you. Plus many build scripts will be totally fine with just the built in rules.

@nedtwigg
Copy link
Member Author

nedtwigg commented Nov 1, 2016

After these preliminary explorations, I agree that most users won't need it, but I like having a quick shortcut available to users who want it Also, as implemented in eefdc50, I think that even if users don't call bumpThisNumberIfRulesChange(), then custom rules will still have good incremental behavior for --continuous mode, which is a neat feature.

@nedtwigg nedtwigg added this to the 3.x milestone Jan 3, 2017
@nedtwigg
Copy link
Member Author

nedtwigg commented Jan 3, 2017

Final name for this rule is bumpThisNumberIfACustomStepChanges. Integration test works on Windows, fails on Unix. Dunno why. Gonna open a separate bug for this to get 3.x out the door.

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