Conversation
|
Yeah, you’re not gonna sneak that one in without a test. 😄 |
Regex was too greedy, matched last instance of what looks like an edition isntead of the first.
|
Our build system seems unhappy about Capitalized Case. |
|
Uh, yeah, as should have been @TheSeeker’s IDE?! Because that’s not valid Java… |
fix Switch/Case -> switch/case
fix missing parens.
fix scope issue.
Bombe
left a comment
Bombe
left a comment
There was a problem hiding this comment.
And the test is still missing! 🙂
| // extract the key type | ||
| String keyType = initialKey.substring(1,3); | ||
| String key = '/' + initialKey + (initialKey.endsWith("/") ? "" : "/"); | ||
| Matcher match = null; |
There was a problem hiding this comment.
Seeing that match is not used anywhere outside the two branches within the switch statement, I believe “fix scope issue” and this change are diametrally opposed. 😀
There was a problem hiding this comment.
Yes, but the switch branches share the scope, so that’s how it has to be to avoid inventing new names for the different branches.
| String initialKey = item.getKey(); | ||
| String key = '/' + initialKey + (initialKey.endsWith("/") ? "" : "/") + "activelink.png"; | ||
| // extract the key type | ||
| String keyType = initialKey.substring(1,3); |
There was a problem hiding this comment.
this has to go from 0 to 3 — Java is zero-indexed.
| switch (keyType) { | ||
| case "USK": | ||
| case "SSK": | ||
| match = PATTERN_USK_SSK.matcher(key); |
There was a problem hiding this comment.
this only matches when the key is already well-formed ⇒ ideally extract to a static method (uses no this; just put static before the function name to compiler-check that), and write a test that takes the existing bookmarks as input (and maybe some bad but valid examples) and returns the correct key for the activelink.
|
I agree very much that we need this fix (thank you!), but the current implementation is still buggy. A test just for the key adjustment should make this much easier to develop. |
3 participants
Bookmarks may not point to the root of a site, so use regex to construct the activelink instead of blindly tacking activelink.png at the end.
This assumes activelinks are always in the root.