-
Notifications
You must be signed in to change notification settings - Fork 57
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
[MM-42669] Avoid superfluous error messages on some autolinks #201
Conversation
Codecov ReportBase: 40.79% // Head: 40.65% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #201 +/- ##
==========================================
- Coverage 40.79% 40.65% -0.15%
==========================================
Files 6 6
Lines 679 674 -5
==========================================
- Hits 277 274 -3
+ Misses 382 380 -2
Partials 20 20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
||
if changed { | ||
post.Message = message | ||
post.Hashtags, _ = model.ParseHashtags(message) |
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 is now inside the if, looks like something that makes sense, because if I haven't change it, the original post should be having properly processes the hashtags already, but is a subtle change in the behavior and I just want to let you know to ensure that it is on purpose change.
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.
Exactly. I think it was basically a bug/redundant before.
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.
LGTM, really nice improvement in the code itself.
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.
Very nice rewrite, LGTM 👍
@DHaussermann 2/5 skip the QA on the PR, but do some release testing around "autolinks", i.e. URLs mixed into the text. |
Let's merge this and test it in release testing |
Summary
For historical reasons, the plugin was skipping autolinks that started with www and were not canonically formatted; it was also LogError-ing unnecessarily.
Also improved the code typography.
Ticket Link
#188
https://mattermost.atlassian.net/browse/MM-42669