-
Notifications
You must be signed in to change notification settings - Fork 938
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
Include Mozilla public suffix list file directly #571
Conversation
Hello @massdosage FWIW, The test set up is also problematic because it leaks a file handle. We should be using a try-with-resource here: // Create a matcher using a custom crafted public suffix list file
try (InputStream in = classLoader.getResourceAsStream(SOURCE_FILE)) {
Assertions.assertNotNull(in);
final List<PublicSuffixList> lists = PublicSuffixListParser.INSTANCE.parseByType(new InputStreamReader(in, StandardCharsets.UTF_8));
matcher = new PublicSuffixMatcher(lists);
} I fixed the above resource leak in git master. The issue remains with the odd test failure on my end though :-( |
OK, the I'm happy to make any changes you suggest here though. |
@@ -40,8 +42,7 @@ | |||
class TestPublicSuffixMatcher { | |||
|
|||
private static final String SOURCE_FILE = "suffixlistmatcher.txt"; | |||
private static final String PUBLIC_SUFFIX_LIST_FILE = "mozilla/public-suffix-list.txt"; |
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.
@massdosage That notation should work on all platforms. I am quite sure of it. This is not the right fix.
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.
OK, yeah, it's been ages since I touched Windows so I wasn't sure. In much earlier versions of Java this would have been an issue. If we instead prefer including the mozilla file in this repo like we do with SOURCE_FILE
I can adjust this PR to do that instead?
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've made that change here now.
Feel free to close this one and use #574 instead. |
As discussed on the dev mailing list, this should fix the failing
TestPublicSuffixMatcher
tests on Windows.