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

dice: clean up the dice "choose" command output (fix #1420) #1425

Merged
merged 2 commits into from
Feb 8, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Dec 28, 2018

See issue #1420 and PR #1421 that I used as my starting point.

Following my comment on #1420 I decided to go with a PR that:

  • strip the space from the input,
  • properly format delimiter in the ouput,
  • add some test example while I was at it,
  • and use the space \s as a last-resort delimiter, because why not,

which improve coverage and make for a better output.

EDIT: we discussed another option, which I implemented afterward, to use comma as an output delimiter in all cases and to add quotation marks when needed.

Example with comma ,:

[nick] .choose a, b, c
[Sopel] Your options: a, b, c. My choice: b

Example with comma , (without spaces):

[nick] .choose a,b,c
[Sopel] Your options: a, b, c. My choice: b

Example with pipe |:

[nick] .choose a | b | c
[Sopel] Your options: a, b, c. My choice: b

Example with pipe | (without spaces):

[nick] .choose a|b|c
[Sopel] Your options: a, b, c. My choice: b

Example with space \s:

[nick] .choose a b c
[Sopel] Your options: a, b, c. My choice: b

Example with pipe | but there are comma in some options:

[nick] .choose a, b and c | a, b | just a
[Sopel] Your options: "a, b and c", "a, b", just a. My choice: a, b

@Exirel Exirel changed the title Clean up the dice "choose" command output (fix #1420) dice: clean up the dice "choose" command output (fix #1420) Dec 28, 2018
@dgw dgw mentioned this pull request Dec 30, 2018
2 tasks
@dgw dgw added this to the 7.0.0 milestone Dec 30, 2018
@dgw dgw added the Tweak label Dec 30, 2018
@dgw
Copy link
Member

dgw commented Dec 30, 2018

This looks good so far! I'm just going to leave a note, for now, but it doesn't need to be addressed immediately. It can wait until after Sopel 6.6.0 is (finally) released.

Commit fb4eb44 duplicates what #1421 does, just in a slightly different way. This PR should be amended (eventually) to make sure only one strip() operation survives into Sopel 7.

@Exirel
Copy link
Contributor Author

Exirel commented Dec 30, 2018

Yes indeed! It'll be done.

@Exirel Exirel force-pushed the dice-choose-output-messy branch from 233b274 to 2d8c0fd Compare December 30, 2018 13:47
@Exirel
Copy link
Contributor Author

Exirel commented Dec 30, 2018

First rebase, just to clean up the split thing. The second one will change the output delimiter to a comma, with option quoted when needed.

@Exirel Exirel force-pushed the dice-choose-output-messy branch from 2d8c0fd to 358be32 Compare December 30, 2018 14:26
@Exirel
Copy link
Contributor Author

Exirel commented Dec 30, 2018

Second rebase, to change the output delimiter to the discussed one (only comma, quote when needed).

@dgw it should be good now!

The `.choose` command's output adds a space after the delimiter: this
works fine after a comma, but not with other delimiters (|, /, and \).

Instead, we use only the comma as a delimiter, and to prevent
ambiguity, we add quotation marks when needed:

    [nick]  .choose a|b|c
    [Sopel] Your options: a, b, c. My choice: a
    [nick]  .choose a,b,c
    [Sopel] Your options: a, b, c. My choice: c
    [nick] .choose a, b | just a
    [Sopel] Your options: "a, b", just a. My choice: a, b
It is now possible to use the space as a delimiter:

    [nick] .choose a b c
    [Sopel] Your options: a, b, c. My choice: c

It is a last-resort delimiter.
@Exirel Exirel force-pushed the dice-choose-output-messy branch from 358be32 to c521b97 Compare January 10, 2019 17:38
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

So, I did manage to trip this up with quoted input:

<dgw>   .ch "foo, bar", spam, "egg, bacon"
<Sopel> dgw: Your options: "foo, bar", spam, "egg, bacon". My choice: "egg

But… I don't think handling that kind of case is worth it. This is actually exactly why the module supports so many delimiters. Using something other than , between items works just fine:

<dgw>   .ch "foo, bar"|spam|"egg, bacon"
<Sopel> dgw: Your options: ""foo, bar"", spam, ""egg, bacon"". My choice: "foo, bar"

The nested quotes look a little weird, admittedly, but this is going to be an exceedingly rare case. So I'm not going to mind it. We can always come back to this function post-7.0.0 if it turns out to be an issue.

@dgw dgw merged commit 42c7ba5 into sopel-irc:master Feb 8, 2019
@Exirel Exirel deleted the dice-choose-output-messy branch February 8, 2019 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants