-
Notifications
You must be signed in to change notification settings - Fork 20
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
deprecate the concept of "codes" (almost) entirely #690
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
79ca6cc
deprecate the concept of "codes" (almost) entirely
artoonie 1a1f0a3
Merge branch 'develop' into feature/issue-663_deprecate-codes
HEdingfield 2e74d82
Cleans up additional references to candidate code.
HEdingfield 3f121b7
don't getNameForCandidate in ResultsWriter
artoonie daa7859
Revert "don't getNameForCandidate in ResultsWriter"
artoonie 7a8e8aa
Merge branch 'develop' into feature/issue-663_deprecate-codes
HEdingfield File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
In this comment, you mentioned possibly reverting / getting rid of some lines in
TabulatorSession
. Is that no longer feasible?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 pushed a commit showing the effect of that. Depends on what we want -- we can get rid of
getNameForCandidate
inResultsWriter
, which generates results that use the candidate code instead of the candidate name. See the diff here.I'm not sure how the
_expected.csv
s are used, so I don't know which format is more useful, but it seems likely that it's more useful with canonical names (i.e. reverting #3f121b7, the next commit) and keeping the code as it was.Let me know which you prefer.
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
_expected.csv
s are just used for our tests.Hard for me to answer this question... might be best for @tarheel or @chughes297 to weigh in? It appears that with the above commit you linked, it leaves our tests as-is, which is probably preferred?
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.
Are the outputted CSVs never used by election administrators? If so, then I suppose it doesn't matter.
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 real output CSV are definitely used by admins. @HEdingfield is just referring specifically to the
_expected.csv
files that are in the test directories.Apologies if I missed a relevant part of the discussion -- I'm just looking at this comment thread -- but I think ideally the output CSV would have the canonical names in one column and then perhaps a separate column that lists any alias that are used for each candidate (separated by a delimiter).
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.
Great. I think that makes sense too. I reverted the last commit, and this PR should be ready to go now.
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.
@chughes297 @tarheel do y'all think it's worth creating a new issue for this? If so, is it something we'd need to include in 1.4.0?
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.
Yes, I think it would be useful to list aliases in the output spreadsheet. I doubt it's a requirement for v1.4, but @chughes297 can comment.
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.
Yes let's create a separate issue but not a huge priority for me to include in 1.4.0.
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.
Created #705 for this.