Allow lookup on URLs #316
Allow lookup on URLs #316
Conversation
Complies with metafacture-core. - increase max value of ClassDataAbstractionCoupling in checkstyle - implement test
| @@ -24,7 +24,9 @@ | |||
| <module name="CatchParameterName"> | |||
| <property name="format" value="^e$"/> | |||
| </module> | |||
| <module name="ClassDataAbstractionCoupling"/> | |||
| <module name="ClassDataAbstractionCoupling"> | |||
| <property name="max" value="8"/> | |||
There was a problem hiding this comment.
That's one way to do it, sure. But then you should also review where this check has been disabled previously and whether these suppressions can be removed.
Alternatively, disable the check where it's failing now. We're going to review the whole Checkstyle configuration at some point.
There was a problem hiding this comment.
Good advise - wasn't aware of that possibility. Disabled the check.
There was a problem hiding this comment.
Disabled the check.
I'm not sure I understand. The ClassDataAbstractionCoupling is still enabled in Metafix.java. And the configuration change hasn't been reverted.
There was a problem hiding this comment.
Hm, right, wasn't commited. Double checked the new PR #317.
| if (path.startsWith(".")) { | ||
| if (fixFile != null) { | ||
| basePath = getPath(fixFile).getParent(); | ||
| if (isValidUrl(path)) { |
There was a problem hiding this comment.
Why not assign resolvedPath here?
There was a problem hiding this comment.
I followed your advice below to declare String resolvedPath; as final. That demands the use of path to not have an initialized variable.
There was a problem hiding this comment.
That demands the use of
pathto not have an initialized variable.
Can you elaborate? There is no change w.r.t. the path variable.
There was a problem hiding this comment.
I cannot assign final String resolvedPath = path to use resolvedPath in if (isValidUrl(path)) {, right? Maybe I just don't understand what you meant here.
| urlPattern = WireMock.urlPathEqualTo(CSV_PATH); | ||
| responseBody = loadFile(CSV_PATH); |
There was a problem hiding this comment.
Why not give these variables appropriate names?
There was a problem hiding this comment.
OK. I've tried to do this , also not reusing Objects. I hope that was your intention here.
There was a problem hiding this comment.
Yes, thanks, that's what I had in mind. Although I don't understand this part:
also not reusing Objects.
There was a problem hiding this comment.
In eef131a both objects urlPattern and responseBody where reused. That made the naming rather generic.
| WIRE_MOCK_SERVER.stubFor(WireMock.get(urlPatternRedirectToUrl) | ||
| .willReturn(WireMock.aResponse() | ||
| .withHeader("Content-Type", "text/turtle") | ||
| .withBody(responseBody))); | ||
|
|
||
| //stub for CSV |
There was a problem hiding this comment.
Minor: Missing space after comment marker.
There was a problem hiding this comment.
Added a checkstyle rule an btw updated this dependency. This brought up some code commented out which I did in part just removed and in part left it (just "fixing" the structure the new checkstyle rule demands) as these comments may have some worth. I didn't find any remarks in git commits so I leave this to you to decide. Actually I don't know if you appreciate the new checkstyle rule at all or want to dump that.
There was a problem hiding this comment.
Actually I don't know if you appreciate the new checkstyle rule at all or want to dump that.
In fact, I don't, sorry. Exactly because it can't differentiate between comments and commented-out code. Some things just have to be left to human judgement ;)
- Check if a whitespace is followed when doing '//' comments - update checkstyle dependency which is last java 8 compatible one
Following 316#pullrequestreview-1495514063.
Complements 48ee14b.
|
I opened new #317 to consolidate commits into one. |
|
Why am I assigned here? Is this PR superseded by #317 or not? |
|
I assigned you because of some new comments from me here I though you may close by clicking on |
Resolves #301.
Complies with metafacture-core.