Use beautifulsoup4 instead of lxml for URL previews
#19301
Conversation
MadLittleMods
changed the title
beautifulsoup4 instead of lxml for URL previews
changelog.d/19301.misc
Outdated
| @@ -0,0 +1 @@ | |||
| Switch to beautofulsoup4 from lxml for URL previews. Controbuted by @clokep. No newline at end of file | |||
There was a problem hiding this comment.
Conflicts to resolve
changelog.d/19301.misc
Outdated
| @@ -0,0 +1 @@ | |||
| Switch to beautofulsoup4 from lxml for URL previews. Controbuted by @clokep. No newline at end of file | |||
There was a problem hiding this comment.
Linting/CI is not passing for typechecking ❌: https://github.com/element-hq/synapse/actions/runs/20170566919/job/57904924809?pr=19301
There was a problem hiding this comment.
Yes, I can't see to get the same setup locally. Did something change with how to install the pinned packages?
There was a problem hiding this comment.
poetry install --extras all is what I use.
Things have changed behind the scenes but shouldn't affect how you install as a developer:
There was a problem hiding this comment.
That's what I did. Maybe I'll try creating a new virtualenv. 🤔
| return None | ||
| tag = soup.find( | ||
| "link", | ||
| rel=("alternate", "alternative"), |
There was a problem hiding this comment.
Where can I find this syntax?
I've looked through https://beautiful-soup-4.readthedocs.io/en/latest/#searching-the-tree
| ), | ||
| # Check microdata for an image. | ||
| meta_image = soup.find( | ||
| "meta", itemprop=re.compile("image", re.I), content=NON_BLANK |
There was a problem hiding this comment.
Why itemprop?
Seems like we could do the normal image=re.I
There was a problem hiding this comment.
itemprop is the key, image is the value.
There was a problem hiding this comment.
So it's more obvious, can you share some example HTML that we're parsing?
| title = soup.find(("title", "h1", "h2", "h3"), string=True) # type: ignore[call-overload] | ||
| if title and title.string: | ||
| og["og:title"] = title.string.strip() |
There was a problem hiding this comment.
I assume we have tests to ensure this does the correct thing? string=True -> title.string and we end up with the title/heading content
|
|
||
| if tree is None: | ||
| return | ||
| from bs4.element import NavigableString, Tag |
There was a problem hiding this comment.
Special reason for organizing the imports here?
Can we do it at the top like normal?
There was a problem hiding this comment.
No, it is an optional dependency. This was the same for lxml.
| if len(elements) > stack_limit: | ||
| # We've hit our limit for working memory | ||
| break |
There was a problem hiding this comment.
Why don't we care about this in the new implementation?
2 participants
Use
beautifulsoup4instead oflxmlfor URL previews. This offers some nicer APIs when parsing HTML and avoids usinglibxml, which is unmaintained.I haven’t done a full regression against commonly previewed sites, but I expect this will give similar (or better) results.
beautiulsoup also handles decoding the charset for us, which is less custom code.