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

Implement vote counts for RFCs #55

Merged
merged 3 commits into from May 27, 2016
Merged

Implement vote counts for RFCs #55

merged 3 commits into from May 27, 2016

Conversation

sgolemon
Copy link
Contributor

Example:
!!rfc typed-properties

[tag:rfc-vote] Accept typed properties? Yes (38: 84%), No (7: 15%)

Example:
  !!rfc typed-properties

  [tag:rfc-vote] [Accept typed properties?](https://wiki.php.net/rfc/typed-properties#doodle__form__accept_typed_properties) Yes (38: 84%), No (7: 15%)
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @kelunik, @DaveRandom and @PeeHaa to be potential reviewers

implode("\n", $messages)
);

$pinnedMessages = yield $this->chatClient->getPinnedMessages($command->getRoom());
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 last bit is just copy-pasta, as I'm not familiar with all the inner workings of the bot. Hopefully it's not terrible.

dogscience

return [new PluginCommandEndpoint('Search', [$this, 'search'], 'rfcs')];
return [
new PluginCommandEndpoint('Search', [$this, 'search'], 'rfcs'),
new PluginCommandEndpoint('RFC', [$this, 'getRFC'], 'rfc'),
Copy link
Member

Choose a reason for hiding this comment

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

For multiple endpoints it really needs a description in the 4th argument here for presentation reasons, but since the API is currently entirely undocumented I'll let you off :-D

@DaveRandom
Copy link
Member

There are a couple of things that need updating here which I'll do this afternoon, but this looks great, thanks!

I'll try and do this correctly but (like everyone else on earth) I don't understand git, or github, so I would appreciate some pointers if anyone knows how to make it work in such a way that I can add commits to this PR and then merge it so that github understands what I did and closes the PR as "merged"?

@kelunik
Copy link
Contributor

kelunik commented May 24, 2016

@DaveRandom You need access to sgolemon:rfc.votes for that I think.

@DaveRandom
Copy link
Member

@kelunik I think I can just add the remote and get a local copy of that branch and merge it locally then github figures it out from the merge commit, I've done it before and it worked but I've also screwed it up before as well

@sgolemon
Copy link
Contributor Author

git add sgolemon https://github.com/sgolemon/Jeeves
git fetch sgolemon
git checkout rfc.votes
# hack hack hack
git commit -a -m "Make more awesomer"
git push origin rfc.votes # origin being git@github.com/DaveRandom/Jeeves

Or just give me a vague idea and I can push in some changes myself.

@sgolemon
Copy link
Contributor Author

I was also thinking of implementing this in a single command, such that the first line below lists RFCs currently in voting, while the second line provides the active vote breakdown. Cleaner as it doesn't introduce another command to muddy up the main list, but I got frustrated trying to sort out if there already was a sub-command concept in here, so I went with the simple approach.

!!rfcs
!!rfcs typed-properties

@DaveRandom
Copy link
Member

Various things do have sub-commands but it's implemented manually from Command#hasArguments()/getArguments() etc - which are just wrappers over an array that was created by splitting on \s+. It's not super clean but the variation in what commands do with extra data is so wide that any further pre-processing would likely be wasteful 95% of the time, but certainly open to suggestions :-)

Now I look at the code again, all that really needs to be done is remove everything after https://github.com/Room-11/Jeeves/pull/55/files#diff-387d128087476a2f9abdd08b138c168bR129 - we don't want to auto-pin this I assume? I was also going to add descriptions strings for the endpoints in the 4th arg(s) at https://github.com/Room-11/Jeeves/pull/55/files#diff-387d128087476a2f9abdd08b138c168bR209 so you can just make the changes yourself then I'll spin it up locally and make sure nothing explodes and that's all that's needed.

If you do want to auto-pin them it's totally doable but would just need a little tweak to the logic - would need a map of rfc -> pinned message id in the storage to make sure that things don't interfere with each other.

@sgolemon
Copy link
Contributor Author

That should do it. And please do spin up a test instance first. I tested the main chunk of the algorithm in isolation, but not the integration points.

@DaveRandom DaveRandom merged commit 94f0312 into Room-11:master May 27, 2016
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