Skip to content
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

898: Dominion CvrExport parsing #899

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

yezr
Copy link
Collaborator

@yezr yezr commented Nov 11, 2024

fixes #898

@yezr yezr linked an issue Nov 11, 2024 that may be closed by this pull request
@yezr yezr changed the title Issue 898: Dominion CvrExport parsing 898: Dominion CvrExport parsing Nov 11, 2024
@artoonie
Copy link
Collaborator

Ha, my bad, should have coordinated this better! I like this solution. One request: can you rename src/test/resources/network/brightspots/rcv/test_data/dominion_multi_file/dominion_multi_file_input_data/CvrExport_0.json to _6.json to test this? And I'll close #900, which was designed to allow a warning for non-sequential files if needed, but perhaps overengineered relative to this solution.

matchedCvrFiles.add(file);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything above, up to the else, can be simplified with:

    String regexPath = CVR_EXPORT_PATTERN.replaceAll("%d", "\\\\d+");
    File cvrDirectory = new File(cvrPath);
    File[] files = cvrDirectory.listFiles((dir, name) -> name.matches(pattern));

which has the added benefit of not duplicating the Cvr_Export string

while (cvrFilePath.toFile().exists()) {
HashMap json = JsonParser.readFromFile(cvrFilePath.toString(), HashMap.class);

for (File file : matchedCvrFiles) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could order of files read matter here? I fear it might with some of the randomizers, but not totally sure if any randomizer works at the CVR level.

For safety, I might sort the matchedCvrFiles just in case it does matter, now or later, so we generate the records in a deterministic order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dominion CvrExport_N.json parsing
2 participants