Manually parse the Ant configUrl field into a URL #4463
Conversation
|
@dimo414 CI must pass. Code style and code coverage are failing. |
|
Sorry, I mistakenly only ran |
|
Hmm, seems like AppVeyor still isn't happy, though I'm not sure why. I can't replicate the failure locally. Any pointers? |
|
appveyor is a windows machine. It is failing on code coverage. Travis has same failure, so most likely you are not getting coverage of your new |
|
Odd, I added |
| setConfigLocation(url.toExternalForm()); | ||
| public void setConfigUrl(String url) { | ||
| try { | ||
| setConfigLocation(new URL(url).toExternalForm()); |
There was a problem hiding this comment.
no need to do instantiation of URL here final URI uri = CommonUtils.getUriByFilename(config); , string could stored as is , to be processed at
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java#L179
conversion to URL/URI - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/utils/CommonUtils.java#L372
There was a problem hiding this comment.
Not clear what you're suggesting - do you want me to use CommonUtils.getUriByFilename() instead, or are you saying setConfigUrl() should simply be:
public void setConfigUrl(String url) {
setConfigLocation(url);
}
?
I think that the latter would be a breaking change for callers that rely on .toExternalForm() being called, but if that doesn't concern you I can change it to that.
There was a problem hiding this comment.
we are decided to go further with breaking compatibility changes, please read #4449 (comment)
|
I've not been able to replicate this on my Windows 10 machine - I run into a number of other seemingly unrelated failures (e.g. in |
|
@dimo414 , please share some logs from your local windows to let us help you. Attention: please read #4449 (comment) |
|
Ok, I've swapped everything to |
| } | ||
| configLocation = location; | ||
| configLocation = file; |
There was a problem hiding this comment.
Please rename configLocation to config so it is seen as a bean.
We are already starting to do this with checks, so reflection utilities will recognize them and tie them together.
There was a problem hiding this comment.
please make method argument to be public void setConfig(String config) {
to make it similar to
public static Configuration loadConfiguration(String config, .... as here
There was a problem hiding this comment.
Done and done - I'd used setConfig(String file) since that's what you suggested in #4449 (comment), but this certainly makes more sense.
|
Travis failures "The forked VM terminated without properly saying goodbye." could be ignored, known instability problem. All other problems need to be resolved. |
|
Please also rebase on latest master, we did travis fix, it should pass. |
Codecov Report
@@ Coverage Diff @@
## master #4463 +/- ##
======================================
Coverage 100% 100%
======================================
Files 285 285
Lines 15323 15317 -6
Branches 3489 3489
======================================
- Hits 15323 15317 -6
Continue to review full report at Codecov.
|
|
@dimo414 you have test failures and checkstyle errors. |
|
Sorry, that was sloppy - I can't get Checkstyle to build on my Windows box so I just made the requested changes. I'll try to fix them. |
did you share with us your build log with problem ? We have CI for Windows build and we have some team members on Windows, all problems resolved a while ago. |
|
Ok, fixed the test failure, but the requested changes (to name the field and the parameter both How would you like me to proceed? Rename one or the other? Or is there a way to suppress this? |
unfortunate case, but public names are above in priority. |
… arbitrary strings, to be consistent with other config logic
|
Ok, |
so that invalid URLs can be treated as resource paths. Fixes #4449.