Add support for remote config file (downloads once per day)

[2rs2ts]

Add two new keys:

1. org.scalastyle.sbt.PluginKeys.scalastyleConfigUrl. It is an Option[String]. Default None.
2. org.scalastyle.sbt.PluginKeys.scalastyleConfigUrlRefreshTime. It is an Option[Integer]. Default Some(24).

To utilize this feature, set scalastyleConfigUrl in your Build.scala to be the URL at which your config file can be found. Now you will automatically download a new version of the config file remotely, so you keep up to date. By default you will update every 24 hours, but you can change scalastyleConfigUrlRefreshTime to change the rate at which that happens. 0 will have it always pull, and None will have it never pull.

If no local scalastyle-config.xml file is found, or if file was last modified more than scalastyleConfigUrlRefreshTime hours ago, then the URL will be used. The contents of the URL are downloaded to the scalastyle-config.xml. If it is not a correct URL or the file at the URL isn't good XML you will get an error. (Common exceptions are java.net.MalformedUrlException, java.io.FileNotFoundException, and org.xml.sax.SAXParseException.) If the download fails but there's still a local version of the config then things will proceed as normal.

If you want to manually force an update, you can delete your local config file or set scalastyleConfigUrlRefreshTime := 0.

This feature is not enabled by default (because scalastyleConfigUrl := None), so old behavior is preserved.

---

This is in contrast to my other PR which downloads every time. If you are doing that and you don't have a network connection, you won't be able to run scalastyle. You'd have to remember to manually download the config. On the other hand, this version will force you to either commit the scalastyle-config.xml or add it to your .gitignore. I leave both options up as a choice.

[matthewfarwell]

This looks great, and I will have closer look at this when I get back from holiday (in about a week).

However, I have two comments - at first glance, if I specify a config file and a url, which takes precedence - I know you explain it above, but I think we could reuse the existing setting. We do this with the maven plugin. If it looks like a URI, then we treat it as such (so it starts with http:// or file://).

Next, could we make the refreshTime an Option as well? Then None means it doesn't get refreshed, Some(0) each time and Some(1) every hour etc. The default should be Some(24) when the config is a URI.

Thanks.

Matthew Farwell.

[2rs2ts]

@matthewfarwell ---

1. At least in this version I had it such that if you specify scalastyleConfigUrl then the remote fetching behavior will start and you will overwrite your local config file. Is that what you were asking about?
2. Great idea, don't know why I didn't think to do it that way myself. Will do.

[matthewfarwell]

For suggestion 1, I obviously didn't dive deeply enough into the PR. I thought that you would be downloading the URI into target/scalastyle_config.xml or whatever. That way, it wouldn't be checked into your repo, and it would also disappear when you did a clean.

[2rs2ts]

@matthewfarwell I had it download to ./scalastyle_config.xml (in the root) because to my understanding that's the file that contains the rules which are loaded upon running. I don't want to detract from your holiday too much, though, so we can talk more about it when you have time to go over the PR. I definitely want to make sure that my changes cause as little friction for users as possible.

[matthewfarwell]

Ok, I've had a bit more time to look at this, and think about it. I think this is a great addition to the plugin. Thanks for your change to the scalastyleConfigUrlRefreshTime.

At first glance, I would have expected the following behaviour:

1) If I specify the config file as a URI (see below how to know this), then the plugin downloads the url into target/foo.xml, where foo.xml is the last portion of the url. We shouldn't be (potentially) forcing a commit every time we do a build. If the file already exists and was not modified in the last scalastyleConfigUrlRefreshTime hours, then it doesn't get refreshed.

Justification: I think, most of the time, we can rely on the URL being available, and if it isn't, that is a build error. The user has specified a URL after all, which implies that it should be available :-). If it isn't, then they can set the refresh time to a large value or 0. Or update the file in target. For instance, you will almost always want a build error if Jenkins can't access the URL.

Also, we shouldn't be (potentially) forcing a commit every time we do a build. If I overwrite the file in git/svn with my changes then this is wrong - if the file has changed it will force me to commit the changes with my (other) changes. Also, I can't force a refresh of the URL by doing a clean.

Thirdly, I think there is a problem with the download logic in your PR - Let's say I check in the scalastyle_config.xml (by error, for whatever reason). Then, when you update (git fetch, whatever, which can be weeks after), you'll be using my version of the checked in file - it has been updated in the last 24 hours. It won't use the URL. This is true of a jenkins checkout as well. Using target/foo.xml fixes this because it isn't part of source control & most of the time Jenkins will do a clean or a fresh checkout.

2) Following the above, I don't need the setting scalastyleConfigUrl - I can reuse scalastyleConfig. I 'know' it is a URI because it starts with 'file://' or 'http://' or 'https://'.

Justification: to change from a file to a URI, I just need one place. If someone comes up with a use case where we need to have two settings, that is a different matter, but let's keep it as simple as possible for the user. They change the file, and things just work.

3) Following the above, we should keep the setting scalastyleConfigUrlRefreshTime - this defaults to a sensible value - your idea of 24 hours is great. Should we rename it to scalastyleConfigUrlRefreshHours ?

Justification: If we put Hours in the name of the setting, the user doesn't have to refer to documentation to know the units to use.

4) There aren't any tests - we need to add some tests to check this functionality. Either you can do this - add some tests into src/sbt-test, copy some of the existing ones, or I can add some tests.

Does this all make sense? What I would suggest is that we discuss and fix the issues under this PR - continue checking in the branch you've made. Then, when we're ready, we can do a new PR with a squash, so that there is a single commit rather than 11.

Again, thanks for your help.

[2rs2ts]

Sounds good to me, I think those are all very reasonable. I will implement the behaviors you expected and add some tests.

I haven't looked at this code for over a week but if I recall correctly I'll have to modify the plugin to look for target/scalastyle-config.xml if it's downloading, though. Users have been putting scalastyle-config.xml in their project root (I know I have) to get the plugin to work, so I don't think they should be expected to move that file. You'd expect that the plugin would use the downloaded file in target/ if scalastyleConfig was a URI, and otherwise it would use the given path, right?

[matthewfarwell]

Yes, exactly. If you specify the URL (http://foo.com/url/foo.xml), it will look in target/xxx.xml, otherwise it is the given path.

[2rs2ts]

@matthewfarwell So, I have the change to use one setting and detect if it's a URI staged, but there's already the config setting which is a File. I'm having the scalastyleConfig setting take precedence if it's set to something other than None for now, but I'm wondering if you want to have that backwards compatibility.

One thing that I think is a good argument for taking out the config setting is that, at my workplace, our build imports sbt._ as well as import org.scalastyle.sbt.PluginKeys._, and this means there's an ambiguous import on "config". I bet it would be good to take out config entirely and have users just change their builds when upgrading to the new version of this plugin. But it's your call.

[matthewfarwell]

This is looking good. I'll have a look at it this weekend.

[matthewfarwell]

Can you rebase this against master, squash it to one commit and then I'll merge it - thanks.

[2rs2ts]

@matthewfarwell Squashed. That was my first time doing a squash so I hope I did it right!

[matthewfarwell]

Cool. Thanks for your work.