-
Notifications
You must be signed in to change notification settings - Fork 120
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 message.go #147
Update message.go #147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you for your contribution.
Do you have any example scenarios where a method like this would be useful? It seems very specific, and may be better suited in your own codebase, as a custom filter. The aim of the filters package is to provide commonly used filters, not to to provide every possible filter.
As I've already mentioned in your previous PR, it is good practice to discuss a feature in the support group, or in github issues, before opening up a PR. This makes it clearer what the intent of the PR is, and allows everyone to build on a shared understanding of the situation.
As it stands, it is unclear to me what the end goal of this PR is; you created a method called MessageEntityPre
, however it doesn't check for pre
entities. Additionally, checking for pre
entities can already be done by the Entity
and CaptionEntity
filters.
@@ -306,3 +306,15 @@ func TopicAction(msg *gotgbot.Message) bool { | |||
return TopicEdited(msg) || TopicCreated(msg) || | |||
TopicClosed(msg) || TopicReopened(msg) | |||
} | |||
|
|||
func MessageEntityPre(offset int, length int, language string) filters.Filter { | |||
return func(ctx *gotgbot.CallbackContext) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function MessageEntityPre
that takes offset
, length
, and language
parameters. However, filters.Filter
func MessageEntityPre(offset int, length int, language string) filters.Filter { return func(ctx *gotgbot.CallbackContext) bool { return false // Placeholder return value } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no such types as filter.Filter
, or gotgbot.CallbackContext
.
|
||
func MessageEntityPre(offset int, length int, language string) filters.Filter { | ||
return func(ctx *gotgbot.CallbackContext) bool { | ||
entities := ctx.Message.Entities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only work on text messages, and not on media messages; that relies on message.CaptionEntities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about it only working on text messages and not media messages, yeah iam included media messages in below cod
func MessageEntityPre(offset int, length int, language string) filters.Filter {
return func(ctx *gotgbot.CallbackContext) bool {
var entities []*gotgbot.MessageEntity
if ctx.Message.Text != nil {
entities = ctx.Message.Entities
} else if ctx.Message.CaptionEntities != nil {
entities = ctx.Message.CaptionEntities
} else {
return false // Neither text nor caption entities are present
}
for _, entity := range entities {
if entity.Offset == offset && entity.Length == length && entity.Language == language {
return true
}
}
return false
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't make code changes in the PR comments, make your code changes to the PR itself. I cant merge a comment :)
return func(ctx *gotgbot.CallbackContext) bool { | ||
entities := ctx.Message.Entities | ||
for _, entity := range entities { | ||
if entity.Offset == offset && entity.Length == length && entity.Language == language { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any example scenarios where one is able to predict both the offset and the length of an incoming message entity? I am not able to come up with one myself.
Additionally, this check is not asserting that the entity is a pre
entity, as suggested by the method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
predicting both the offset and length of an incoming message entity might not always be feasible or practical, especially in real-time scenarios. However, if you're dealing with predefined message formats or templates, you could potentially predict these values based on the structure of your messages.
Here is one example to you understand proper
Let's say you have a system where users submit code snippets in a specific format, and you know that each code snippet starts at a fixed offset and has a fixed length. In this case, you can predict the offset and length of the code snippet entity based on the message format.
However, if the offset and length need to be dynamically determined based on user input or other factors, predicting them accurately might be challenging. In such cases, it's often better to rely on other attributes or patterns in the message content rather than trying to predict specific offsets and lengths.
Regarding the method name not asserting that the entity is a pre entity, you're correct. If you want to specifically filter preformatted code entities, you should include additional checks for the entity type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you've noted yourself, this isnt feasible or practical. Which makes this function very unlikely to be used. Users that will hit the niche usecase you've mentioned can simply implement the filter themselves, to work exactly like they need.
Given this, it sounds like we agree that this PR should not be merged.
here is the example to how to use this
|
@TG-BOTSNETWORK While I appreciate the example, CI is still failing on this PR, which makes me doubt that any of it has been tested |
Closing because filter is too niche to make sense in the library |
which is used to filter preformatted code entities based on their programming language. The filter will return true if it finds a preformatted code entity with the specified language, otherwise false.