Skip to content
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

Embed: replace pyquery with selectolax #7988

Closed
wants to merge 2 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 5, 2021

Don't introduce a new dep.
MkDocs doesn't work since it doesn't have fjson files,
I'm deleting the test for mkdocs for now,
I'll add support for MkDocs once we have the parsing done in a more
general way (soon!).

Based on #7986

Don't introduce a new dep.
MkDocs doesn't work since it doesn't have fjson files,
I'm deleting the test for mkdocs for now,
I'll add support for MkDocs once we have the parsing done in a more
general way (soon!).
@stsewd stsewd force-pushed the replace-pyquery branch from 8c3bd3b to 347d371 Compare March 5, 2021 01:21
@stsewd stsewd requested a review from ericholscher March 8, 2021 18:34
@humitos
Copy link
Member

humitos commented Mar 8, 2021

I don't want to block this work, but I want to say that I don't feel super comfortable with this change considering the low amount of tests that we have and the weak this code has been being all this time.

I'm afraid that we could break all/many/few/random popups on project's production documentation without noticing at all. Basically, I think the cost could be too high and there is almost no benefit on swapping pyquery by selectolax at this point.

@stsewd
Copy link
Member Author

stsewd commented Mar 8, 2021

I don't want to block this work, but I want to say that I don't feel super comfortable with this change considering the low amount of tests that we have and the weak this code has been being all this time.

We already have a bunch of tests #7986

I'm afraid that we could break all/many/few/random popups on project's production documentation without noticing at all. Basically, I think the cost could be too high and there is almost no benefit on swapping pyquery by selectolax at this point.

We are going to be able to share more code from the search parsers, those supporting mkdocs

@ericholscher
Copy link
Member

I agree that we should hold off on this for now. I think we want to think about a way to create a "v3" of the embed API that does a few things:

  • Has a design doc
  • Major changes to backend code
  • Change API response
  • Solid CDN support
  • Has public docs

I think these major breaking changes should likely go into that. We want to keep the old v2 stable while we deprecate it, and move most of this new work into the new API since it will have more thought behind it.

@stsewd stsewd force-pushed the embed-test-def-lists branch 3 times, most recently from 8a0078d to da5eff4 Compare March 17, 2021 23:25
@stsewd
Copy link
Member Author

stsewd commented Mar 18, 2021

In my defense here, the output changing would be very weird as the css selectors are the same and this is changing the html parser used, the parsers don't change the html. We didn't have tests before, so we don't know if any change is going to "break" it (as users rely on html, I don't see that really breaking as opposed to change how we return those elements, which this isn't changing). I do agree on having breaking changes in the response and more docs on v3 since v2 is being already used in the wild.

@humitos
Copy link
Member

humitos commented Mar 29, 2021

I think we should close this PR and start the v3 endpoint using selectolax if that's better. I don't think there are good reasons to change the backend library we are using at this point.

@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jun 2, 2021
@humitos
Copy link
Member

humitos commented Jun 3, 2021

Closing for now. We can come back if we feel it's required.

Related to #8222

@humitos humitos closed this Jun 3, 2021
@stsewd stsewd deleted the replace-pyquery branch June 3, 2021 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: stale Issue will be considered inactive soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants