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

Update acknowledging_requests.md #2086

Merged
merged 2 commits into from Apr 4, 2024

Conversation

technically-tracy
Copy link
Contributor

@technically-tracy technically-tracy commented Apr 4, 2024

Summary

Update regex based on developer feedback from X (formerly Twitter).

Links to feedback:
https://twitter.com/otrejni/status/1625787621057101825
https://twitter.com/otrejni/status/1625790926042943489

Requirements (place an x in each [ ])

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.97%. Comparing base (e5728c0) to head (34d8997).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2086   +/-   ##
=======================================
  Coverage   81.97%   81.97%           
=======================================
  Files          18       18           
  Lines        1531     1531           
  Branches      440      440           
=======================================
  Hits         1255     1255           
  Misses        178      178           
  Partials       98       98           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@technically-tracy technically-tracy added the docs M-T: Documentation work only label Apr 4, 2024
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

Appreciate the change greatly! Regex is always fun to mess with 🤪

Left a small change to help us catch more of those TLDs with a different syntax - emails ending in .coffee or .codes will no longer be missed ☕

docs/_basic/acknowledging_requests.md Outdated Show resolved Hide resolved
@zimeg zimeg added this to the 3.18.0 milestone Apr 4, 2024
Co-authored-by: Ethan Zimbelman <ethan.zimbelman@me.com>
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

🚀 Thanks for finding this fix! Ready to merge whenever IMO but left one optional suggestion on expanding the regex. I can see good argument for not including it ("we only want plain emails") so feel free to ignore it!

docs/_basic/acknowledging_requests.md Show resolved Hide resolved
@technically-tracy technically-tracy merged commit a52e790 into slackapi:main Apr 4, 2024
8 checks passed
seratch added a commit that referenced this pull request Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs M-T: Documentation work only semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants