Skip to content
This repository has been archived by the owner on Mar 13, 2023. It is now read-only.

feat: Text Utils #700

Merged
merged 14 commits into from
Nov 1, 2022
Merged

feat: Text Utils #700

merged 14 commits into from
Nov 1, 2022

Conversation

AlbertUnruh
Copy link
Contributor

What type of pull request is this?

  • Non-breaking code change
  • Breaking code change
  • Documentation change/addition
  • Tests change

Description

Adds a function to check whether something was mentioned somewhere.

Changes

  • add naff.client.utils.text_utils

Checklist

  • I've formatted my code with Black
  • I've ensured my code works on Python 3.10.x
  • I've tested my code

return query.match(text) is not None
elif isinstance(query, models.BaseUser):
# mentions with <@!ID> aren't detected without the replacement
return (query.mention in text.replace("@!", "@")) or (query.username in text)
Copy link
Member

Choose a reason for hiding this comment

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

Why are you checking if the username is in the text? LordOfPolls isn't a mention <@174918559539920897> is

Further, if you removed that superfluous check you could squash this and the below if statements into one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to check for (Discord) mentions, but for the presence itself (if it makes sense).

Copy link
Member

Choose a reason for hiding this comment

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

That seems counter-intuitive to the feature's name, nor how most people would assume it works.

Assuming we did go that route, however, using display_name would be better surely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

*at least I would check for BaseUser.tag -> LordOfPolls#1010

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems counter-intuitive to the feature's name, nor how most people would assume it works.

Assuming we did go that route, however, using display_name would be better surely.

Any other suggestions for the features name?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest removing the username detection and keeping the name as it is. ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Username detection really feels prone to false positives. I definitely wouldn't want it by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like so?

elif isinstance(query, models.BaseUser):
# mentions with <@!ID> aren't detected without the replacement
return (query.mention in text.replace("@!", "@")) or (query.tag in text if const.tag_as_mention else False)

naff/client/const.py Outdated Show resolved Hide resolved
@AlbertUnruh AlbertUnruh requested review from silasary and LordOfPolls and removed request for silasary and LordOfPolls October 31, 2022 14:53
Copy link
Member

@LordOfPolls LordOfPolls left a comment

Choose a reason for hiding this comment

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

Inconsequential bits, otherwise LGTM

naff/client/utils/text_utils.py Outdated Show resolved Hide resolved
naff/models/discord/message.py Outdated Show resolved Hide resolved
naff/client/utils/text_utils.py Outdated Show resolved Hide resolved
@LordOfPolls LordOfPolls merged commit bb33ba2 into NAFTeam:2.x Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants