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

Make Poll create close, change init and make tally #9

Merged
merged 15 commits into from
Nov 30, 2018
Merged

Conversation

FrozenChen
Copy link
Collaborator

@FrozenChen FrozenChen commented Nov 22, 2018

Fixes #6 Fixes #5 Fixes #4 Fixes #2

Note: For any further changes to this branch, please make sure you do not accidentally merge json files, as the gitignore has not been updated on this branch

Copy link
Owner

@GriffinG1 GriffinG1 left a comment

Choose a reason for hiding this comment

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

options @ 47 won't take all options currently, needs *, before the variable declaration
rename create to start and close to end

Other than that it looks good to me, but probably should let @architdate and/or @noirscape take a glance at it too before merging

Copy link
Owner

@GriffinG1 GriffinG1 left a comment

Choose a reason for hiding this comment

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

Would still like archit and/or noir to look over code before merging

Copy link
Collaborator

@noirscape noirscape left a comment

Choose a reason for hiding this comment

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

Left quite a couple of suggestions.

d.py is a powerful framework but uh... a lot of this code really doesn't take advantage of that.

- You're doing error handling for missing arguments in the command itself, there's a dedicated way to do this.
- Group the Poll command.
- Filenames use + for combining strings in a couple places. This is bad form and inconsistent with the earlier use of f-strings to make a string.
- There's a couple of spacing issues in some function calls (no spaces after a comma). I've highlighted a single example.
- is_poll_ongoing() sends text if it fails. Bad form, this should be done through an error handler.
- Tally command doesn't properly wait for the Queue to be empty, use the asyncio.Queue.join() function.

addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
addons/vote.py Outdated Show resolved Hide resolved
@GriffinG1 GriffinG1 mentioned this pull request Nov 22, 2018
Copy link
Collaborator

@noirscape noirscape left a comment

Choose a reason for hiding this comment

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

lgtm although create_cmd still prefills and errors if missing. probably shouldnt prefill those vars.

@GriffinG1
Copy link
Owner

Whups forgot to remove that when I'd removed it sending what was inputted, since I realized that wasn't necessary cause no command deletion.

Uses join() and task_done()
- Poll info during poll is in `poll.json` while votes are in `votename_votes.json`
- Poll info after vote is all moved to `Polls` folder with `poll.json` becoming `votename.json`
- No longer writing empty dicts to files, just remove the damn thing.
- Explicit str casting to prevent double votes
- Swap out all `cmd` functions to their actual yknow, names?
- Added spacing -> Compacted code might look good in low LOC, but it doesnt make it more readable.
- Moved poll_ongoing boolean set to before moving the files, that prevents voters during the file move.
- Poll tallies before closing.
@architdate architdate requested review from architdate and removed request for architdate November 30, 2018 10:44
@architdate architdate merged commit 42acbd9 into master Nov 30, 2018
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.

4 participants