-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace LinkedFiles backslashes with forward slashes #3364
Conversation
Ensures cross platform compatibility
add changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix itself is fine, I just have one remark regarding a refactoring.
* @return True if it starts with http://, https:// or contains www; false otherwise | ||
*/ | ||
public static boolean isOnlineLink(String toCheck) { | ||
return toCheck.startsWith("http://") || toCheck.startsWith("https://") || toCheck.contains("www."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL validation is quite hard (potentially completely infeasible) in practice.
Is this implementation sufficient for our purposes? I mean "jabref.org" is a valid url, but would isOnlineLink()
would return false.
My thoughts are to leave this method in the LinkedFile
if it should serve as a "special purpose" link checker. If it should be a general method, in the util
package, then the implementation would need to become more elaborate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not change the detection of urls, I just moved it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what I mean: if you move it to an util
package, it should be a general purpose method that works in 'all' cases.
If it stays in this class (potentially private) it's sufficient if it is good enough to be called in that specific context.
yes, agree, good point I readded it in the other PR |
Ensures cross platform compatibility
Fixes #3311
Tested this under windows and it works fine
gradle localizationUpdate
?