fix : URI detector/template loading from windows fs #110
Conversation
This reverts commit 9e0bf7e.
dgautier
changed the title
dgautier
force-pushed
the
fix/logging-invalidpathexception
branch
from
1bcbbc3 to
2629949
Compare
| @@ -27,9 +28,10 @@ class AsciidocParser( | |||
| } | |||
|
|
|||
| private val templatesLocation: Path by lazy { | |||
| val templateResources = AsciidocParser::class.java.getResource(TEMPLATES_LOCATION)!!.toURI() | |||
| val templateResources = AsciidocParser::class.java.classLoader.getResource(TEMPLATES_LOCATION)!!.toURI() | |||
There was a problem hiding this comment.
what is the reason for this change?
| @@ -70,6 +71,7 @@ class PageContentTest { | |||
|
|
|||
| @Test | |||
| internal fun `Invalid result for unbalanced xml`() { | |||
| Locale.setDefault(Locale.ENGLISH); | |||
There was a problem hiding this comment.
this is a bad idea - especially taking into account that you do it without any try/finally/extension way.
It's better to use something like this https://wesome.org/junit-5-pioneer-defaultlocale or create adhoc extension in tests to do something similar
| private val logger = KotlinLogging.logger {} | ||
| } | ||
| fun isValid(url: String): Boolean { | ||
| val pattern = Pattern.compile(URI_DETECTOR, Pattern.CASE_INSENSITIVE); |
There was a problem hiding this comment.
please use regex as a field instead to avoid compiling regex string every time. Original code worked better, so please rollback to original approach.
| } | ||
|
|
||
| private val normalizedDocs = | ||
| documents.map { (path, header) -> path.relativeTo(basePath).normalize() to header }.toMap() | ||
|
|
||
| override fun resolveReference(source: Path, refTo: String): Reference? { | ||
| if (URI_DETECTOR.matches(refTo)) return null | ||
|
|
||
| if (refTo.startsWith(MAILTO_DETECTOR)) return null |
There was a problem hiding this comment.
I really don't get what's the reason of doing multiple matches - how about simply slightly adjusting original regex to catch url scheme and checking that it's http, https, ftp or mailto?
|
|
||
| if (refTo.startsWith(MAILTO_DETECTOR)) return null | ||
| if (refTo.startsWith(LOCALHOST_DETECTOR)) return null | ||
| if (isValid(refTo)) return null |
There was a problem hiding this comment.
method name is confusing - what does "valid" mean here? better to rename with isExternalUrl or something like this
| val document = normalizedDocs[targetPath]?.title ?: return null | ||
| return Xref(document, anchor) | ||
| } catch (ex: InvalidPathException) { | ||
| logger.error { "Failed to resolve : $refTo from $source" } |
There was a problem hiding this comment.
it's a bad pattern - log error and rethrow exception - because you'd get one more logging in another place. Either do not log or throw another exception with chained root cause.
|
|
||
|
|
||
| @Test | ||
| internal fun `Http resolution`() { |
There was a problem hiding this comment.
PR title says that there is some fix for windows. What actually is failing here on windows?
|
Fixed in #116. Differences:
|
No description provided.