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

Check for whitespace at beginning of a watch pattern, requiring -force to continue #6360

Merged
merged 3 commits into from Jun 6, 2021

Conversation

Spevacus
Copy link
Contributor

@Spevacus Spevacus commented May 12, 2021

I recently accidentally watched an expression with a space at the beginning completely unintentionally, as I was copy-pasting off of a list. In chat, this looks as though everything worked correctly and there is nothing wrong, but there's actually some hidden leading whitespace there.

This isn't the first time I've run into this problem. I've approved #5921 and #5789 (knowingly, I'll add) just to see if they would work, because the watch commands as viewed from chat would lead one to believe it was entered correctly. Naturally, the watches did not catch their intended targets in these instances.

What this PR does:
This addition checks for leading white space against the currently proposed pattern. If it's found, it throws a similar message at the user that #5114 introduced, and allows -force to be used to override.

I know of no real reason why anyone would want to start a watch with whitespace, but given that SE chat shaves off leading whitespace visually (but does not functionally) it's 99 times out of 100 a mistake, and in my opinion, should warn the user about what they're doing while still preserving the ability to force the change through if thou happen-est to be that 1 user out of 100.

Potential improvements:

  • Implement something akin to what Makyen explains here, which would probably be more useful to the user.
  • Display the raw pattern to the user in the reply, so they can see the source of the message they sent, and thus why Smokey found it problematic.

Did you even test this?
Yep, I did! I didn't have a spare sock so I just threw this against my own user and fired away. As a note, for this test there were two differences. One, I used pattern.startswith(" "), which I changed to use regex considering #5928 did something similar and used regex (and this regex covers various whitespace characters, not just a "regular" space.) Two, the error message was very slightly different, and even then, only in punctuation and grammar (looks cleaner now, imo)

Feel free to adjust this PR as fitting with the code practices used by Charcoalers, or request that I do so.

@Spevacus
Copy link
Contributor Author

For completeness, I tested the regex version (as I mentioned previously the detection was similar during my prior testing) with Xnero in this session. Seemed to work fine.

@makyen
Copy link
Contributor

makyen commented Jun 6, 2021

The combining of multiple reject reasons is something which we should rewrite, as it doesn't really produce ideal output in chat. However, that's not something which this PR really needs to fix.

@makyen makyen merged commit 7bdff77 into Charcoal-SE:master Jun 6, 2021
@Spevacus Spevacus deleted the patch-1 branch June 6, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants