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

Add command to skip without downvoting #17

Merged
merged 4 commits into from
Feb 24, 2017
Merged

Add command to skip without downvoting #17

merged 4 commits into from
Feb 24, 2017

Conversation

rjanja
Copy link

@rjanja rjanja commented Feb 24, 2017

Also "what's next", cleans up some help, minor refactoring

send message, response
end

hear ~r/^dj\s(what|who).*\snext$/i, message do
Copy link
Member

Choose a reason for hiding this comment

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

This call is not in the help, and its regex is too generic.

Copy link
Author

Choose a reason for hiding this comment

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

I've added help for this command. As far as the regex being too generic, I've reused what's already there for what/who's playing. We can create a new issue for being more specific in both commands if desired

Copy link
Member

Choose a reason for hiding this comment

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

I don't see why we'd want to repeat the sins of our ancestors on new code.

`dj (what|who) ('s| is) playing` displays the currently playing track and playlist (e.g. what's playing or who is playing)
`dj +1|:thumbsup:|:thumbsup_all:|:metal:|:shaka:|up|yes` upvotes if you like the currently playing track on the currently playing playlist
`dj -1|:thumbsdown:|:thumbsdown_all:|down|no|skip|next` votes against the currently playing track on the currently playing playlist and skips to the next track
`dj who's playing` or what's playing, shows track and playlist
Copy link
Member

Choose a reason for hiding this comment

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

The old documentation is correct, this looses other supported requests.

Copy link
Author

Choose a reason for hiding this comment

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

Emojis are not rendered within backticks, so we should either get rid of the code-formatted help or avoid specifying emoji. Here I've chosen the latter.

I would argue that a global help that shows all possible forms of every command is not that helpful and just becomes difficult to read.

response = with {:ok, level} <- Mixer.get_volume do
"The volume level is set to #{round(level/10)}"
end
send message, response
end

hear ~r/^dj\svolume\s(?<level>.*)$/i, message do
hear ~r/^dj\svol(ume)?\s(?<level>.*)$/i, message do
Copy link
Member

Choose a reason for hiding this comment

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

Should be included in the help.

Copy link
Author

Choose a reason for hiding this comment

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

Do we really need to tip the reader that they can either spell out volume or not? It should do the same thing

@rjanja
Copy link
Author

rjanja commented Feb 24, 2017

Closes #3

@holetse holetse merged commit 4eee8e4 into master Feb 24, 2017
@holetse holetse deleted the skip-only branch February 24, 2017 04:16
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.

2 participants