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

Cert bug fixes #287

Merged
merged 20 commits into from
Jun 19, 2019
Merged

Cert bug fixes #287

merged 20 commits into from
Jun 19, 2019

Conversation

moldover
Copy link
Contributor

This contains fixes for #278 #141 and #271
I generally tried to improve the log messaging and surface issues and status in a clearer and more consistent way.

Copy link
Contributor

@tarheel tarheel left a comment

Choose a reason for hiding this comment

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

It all looks good to me. I'll give @HEdingfield a chance to review also.

src/main/java/com/rcv/Logger.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/FileUtils.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/Main.java Outdated Show resolved Hide resolved
# Conflicts:
#	src/main/java/network/brightspots/rcv/GuiApplication.java
#	src/main/java/network/brightspots/rcv/Logger.java
#	src/main/java/network/brightspots/rcv/TabulatorSession.java
#	src/main/java/network/brightspots/rcv/TieBreak.java
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.

I hope my comments appear, but they might not since it seems like you made changes when I was in the process of commenting. ARGH GITHUB!!!!!!

src/main/java/com/rcv/GuiApplication.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/TieBreak.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/FileUtils.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/Main.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/GuiConfigController.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/Tabulator.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/ContestConfig.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/Main.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/Main.java Outdated Show resolved Hide resolved
src/main/java/com/rcv/Main.java Outdated Show resolved Hide resolved
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.

Nice, getting close! Went through everything and made some more suggestions (did them as suggestions so you can accept them right in the GH comments hopefully).

A couple of other important things to address:

  • There are around 20-30 files that still look stuck in com/rcv, so this PR is currently attempting to move them back from network/brightspots/rcv... need to fix this.
  • Try deleting these: .idea/_artifacts, idea/artifacts, .idea/kotlinc.xml. I'm 95% sure these are just leftovers from pre-Gradle days and no longer serve any purpose.

src/main/java/network/brightspots/rcv/ContestConfig.java Outdated Show resolved Hide resolved
src/main/java/network/brightspots/rcv/Main.java Outdated Show resolved Hide resolved
src/main/java/network/brightspots/rcv/Main.java Outdated Show resolved Hide resolved
src/main/java/network/brightspots/rcv/Tabulator.java Outdated Show resolved Hide resolved
src/main/java/network/brightspots/rcv/Tabulator.java Outdated Show resolved Hide resolved
@HEdingfield HEdingfield changed the base branch from master to develop June 18, 2019 16:24
moldover and others added 10 commits June 18, 2019 21:19
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com>
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.

Looks great! For future reference, I think you can batch all the suggestions into one commit by using the "Add suggestion to batch" button.

@moldover moldover merged commit f678a4e into develop Jun 19, 2019
@moldover moldover deleted the cert_bug_fixes branch June 19, 2019 14:59
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.

3 participants