-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Bug] Fix regex for urls in image function #928
Conversation
Tag @nking-1 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #928 +/- ##
==========================================
+ Coverage 56.43% 61.43% +4.99%
==========================================
Files 63 63
Lines 4791 4792 +1
==========================================
+ Hits 2704 2944 +240
+ Misses 2087 1848 -239 ☔ View full report in Codecov by Sentry. |
This looks like a solid fix with tests. We'll probably be changing the internals of how images are stored and serialized in the model, but at a high level it shouldn't impact the fixes here. I think we can go ahead and take this change, and if any further work is needed I can do it as part of the upcoming multimodal support feature |
@kklemon , I think you may be hitting this issue in the tests: |
The regex for matching urls in the
image
function starts with a$
character (ref), i.e. the "match end of string"-metacharacter, which seems to be wrong. I guess the intention was to match against the start of the string, which could be achieved with a^
character, but sincere.match
matches against the start of a string already, this can also be omitted in this specific case.This led to the situation that urls are generally not recognized and can't be used with the
image
function. This PR attempts to fix this issue by modifying the aforementioned regex and additionally providing tests for the_image
module to prevent such problems in the future.