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

Remove submodule per #612 #615

Merged
merged 80 commits into from
Aug 15, 2022
Merged

Remove submodule per #612 #615

merged 80 commits into from
Aug 15, 2022

Conversation

moldover
Copy link
Contributor

@moldover moldover commented Aug 12, 2022

Removes test_data submodule and re-add test_data files directly into the main repo as a subtree which preserves history.
#612

@artoonie tagging to keep you in the loop here.

HEdingfield and others added 30 commits June 13, 2019 23:10
…s example config file for testing interactive tiebreaker with sequential multi-seat enabled; adds commented command in build.gradle for executing CLI arguments.
# Conflicts:
#	src/main/java/network/brightspots/rcv/TabulatorSession.java
add residual surplus to overall summary JSON
add a test for the generatePermutation tie-breaking option
HEdingfield and others added 15 commits October 2, 2020 12:47
* Updates checkstyle from 8.34 to 8.36.2, and google_checks.xml.
* Creates and implements separate `checkstyle-suppressions.xml` file to suppress Checkstyle warnings that don't make sense for our project (fixes #489).
* Addresses relevant Checkstyle warnings (fixes #490).
* Addresses IntelliJ warnings.
* Pixel-pushing for Linux (get thicc, boi)
* Changes all logging to use new `fine()`, `info()`, `warning()`, and `severe()` methods.
* Gets rid of now-unused `onEditCommit` functions for the CVR Files and Candidates tables.
* Makes util classes `final`, with private constructors to prevent instantiation.
* Renames `TieBreakMode`, `tieBreakMode`, `TieBreak`, and `tieBreak` to `TiebreakMode`, `tiebreakMode`, `Tiebreak`, and `tiebreak`, respectively. This word has now lost all meaning.
* Removes unnecessary usages of `.toString()`.
* Standardizes exceptions as `exception` instead of `e` to comply with VVSG requirement on variable names (5.2.5 paragraph c) that only index loops are allowed to have single-character variable names.
* Fixes broken test.
* Minor fixes to hints.
* Get rid of the Devilish Double Spaces after periods.
* fix README

* CDF JSON: throw if a CandidateObject can't be found during parsing.

* Cleanup CDF class (type) names as output from ResultsWriter.

* fix label
* Handle missing precinct data for older Dominion data sets #533
* Add test data with missing precinct data for older Dominion data sets #533
* Fix for #536 - allow multiple CDF files
Add regression test with multiple CDF files
# Conflicts:
#	.idea/misc.xml
#	README.md
#	build.gradle
#	config/checkstyle/google_checks.xml
#	config_file_documentation.txt
#	src/main/java/network/brightspots/rcv/CastVoteRecord.java
#	src/main/java/network/brightspots/rcv/CommonDataFormatReader.java
#	src/main/java/network/brightspots/rcv/ContestConfig.java
#	src/main/java/network/brightspots/rcv/DominionCvrReader.java
#	src/main/java/network/brightspots/rcv/FileUtils.java
#	src/main/java/network/brightspots/rcv/GuiConfigController.java
#	src/main/java/network/brightspots/rcv/Logger.java
#	src/main/java/network/brightspots/rcv/Main.java
#	src/main/java/network/brightspots/rcv/ResultsWriter.java
#	src/main/java/network/brightspots/rcv/StreamingCvrReader.java
#	src/main/java/network/brightspots/rcv/Tabulator.java
#	src/main/java/network/brightspots/rcv/TabulatorSession.java
#	src/main/java/network/brightspots/rcv/Tiebreak.java
#	src/main/resources/network/brightspots/rcv/GuiConfigLayout.fxml
#	src/test/java/network/brightspots/rcv/TabulatorTests.java
#	src/test/resources/network/brightspots/rcv/test_data/2013_minneapolis_mayor_scale/2013_minneapolis_mayor_scale_config.json
#	src/test/resources/network/brightspots/rcv/test_data/continue_until_two_with_batch_elimination_test/continue_until_two_with_batch_elimination_test_config.json
#	src/test/resources/network/brightspots/rcv/test_data/multi_seat_bottoms_up_with_threshold/multi_seat_bottoms_up_with_threshold_config.json
#	src/test/resources/network/brightspots/rcv/test_data/multi_seat_uwi_test/multi_seat_uwi_test_config.json
#	src/test/resources/network/brightspots/rcv/test_data/multi_seat_uwi_test/multi_seat_uwi_test_expected_summary.json
#	src/test/resources/network/brightspots/rcv/test_data/precinct_example/precinct_example_config.json
#	src/test/resources/network/brightspots/rcv/test_data/skip_to_next_test/skip_to_next_test_config.json
#	src/test/resources/network/brightspots/rcv/test_data/test_set_1_exhaust_at_overvote/test_set_1_exhaust_at_overvote_config.json
#	src/test/resources/network/brightspots/rcv/test_data/test_set_2_overvote_skip_to_next/test_set_2_overvote_skip_to_next_config.json
#	src/test/resources/network/brightspots/rcv/test_data/test_set_3_skipped_choice_exhaust/test_set_3_skipped_choice_exhaust_config.json
#	src/test/resources/network/brightspots/rcv/test_data/test_set_4_skipped_choice_next/test_set_4_skipped_choice_next_config.json
#	src/test/resources/network/brightspots/rcv/test_data/test_set_6_duplicate_exhaust/test_set_6_duplicate_exhaust_config.json
#	src/test/resources/network/brightspots/rcv/test_data/test_set_multi_winner_fractional_threshold/test_set_multi_winner_fractional_threshold_config.json
#	src/test/resources/network/brightspots/rcv/test_data/test_set_multi_winner_whole_threshold/test_set_multi_winner_whole_threshold_config.json
#	src/test/resources/network/brightspots/rcv/test_data/uwi_cannot_win_test/uwi_cannot_win_test_config.json
#	src/test/resources/network/brightspots/rcv/test_data/uwi_cannot_win_test/uwi_cannot_win_test_expected_summary.json
…it '15f077983ccc5d370bd815e926798ef993758613'

git-subtree-dir: src/test/resources/network/brightspots/rcv/test_data
git-subtree-mainline: 31c2ec8
git-subtree-split: 15f0779
Copy link
Contributor

@HEdingfield HEdingfield left a comment

Choose a reason for hiding this comment

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

Hmm, not sure how you went about doing this, but there are TONS of commits listed in the PR, which is kind of concerning. Any idea why?

@HEdingfield
Copy link
Contributor

I'm guessing related to this?

re-add test_data files directly into the main repo as a subtree which preserves history

Is there a reason that we can't just re-add the files and make the changes as a new commit? I'd prefer not to erase the history of having done this.

@moldover
Copy link
Contributor Author

moldover commented Aug 12, 2022

Hmm, not sure how you went about doing this, but there are TONS of commits listed in the PR, which is kind of concerning. Any idea why?

That's the history being re-created. When the submodule was first created those files became unparented from the original tree and history. This is reapplying that history onto the new base. I don't understand all this in detail, but the history is all preserved (although it's a bit wonky now) but that's what matters. Look at the file history for one of the test_data files and you'll see what I mean.

I pulled this from:

https://stackoverflow.com/questions/52224200/convert-a-git-submodule-to-a-regular-directory-and-preserve-the-history-in-the-m

@HEdingfield
Copy link
Contributor

Thanks for that context, I think I understand a bit. We basically want to make sure the history of the 3-5 commits made in rcvtests since its inception are re-integrated into rcv's history?

I think that makes sense, but I'd feel most comfortable if @artoonie gives this his close review and approval.

@moldover
Copy link
Contributor Author

moldover commented Aug 12, 2022

It's really making sure ALL the history is integrated. Yeah, actually I wanted him to review it too. I just invited him to the group.

@tamird tamird removed their request for review August 12, 2022 18:01
Copy link
Collaborator

@artoonie artoonie left a comment

Choose a reason for hiding this comment

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

nice git-fu gettting the history back! Looks like git blame does what we want it to do. 👍

@moldover
Copy link
Contributor Author

Sweet. Just waiting on @HEdingfield then.

@@ -2,6 +2,5 @@
<project version="4">
<component name="VcsDirectoryMappings">
<mapping directory="" vcs="Git" />
<mapping directory="$PROJECT_DIR$/src/test/resources/network/brightspots/rcv/test_data" vcs="Git" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why this change is necessary? Not a blocker, but I'm curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tells intelliJ that the submodule is a submodule which enables Intellij vcs integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's pretty solid btw - worth checking out even if you have a git client you like. @

@@ -1,3 +0,0 @@
[submodule "src/test/resources/network/brightspots/rcv/test_data"]
path = src/test/resources/network/brightspots/rcv/test_data
url = https://github.com/brightspots/rcvtests.git
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also mention of "submodules" in the contributor guide here and in the test.yaml file here that might need to be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove the reference from the guide.

The yaml I imagine is just a generic configuration to configure submodules recursively if there are any. It doesn't seem to have impacted the test from running and completing which is the important thing here. @artoonie any thoughts on this?

@moldover moldover merged commit 2acdd92 into develop Aug 15, 2022
@moldover moldover deleted the remove_submodule_612 branch August 15, 2022 19:58
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.

None yet

4 participants