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

RFC: Lazily fetch id and classes #101

Merged
merged 2 commits into from
Mar 3, 2023
Merged

RFC: Lazily fetch id and classes #101

merged 2 commits into from
Mar 3, 2023

Conversation

adamreichold
Copy link
Member

I think this might be a useful addition to #91 by avoiding the upfront costs for all elements and only materializing the set of classes when they are actually used.

This does come with an API change (no more public fields id and classes) and also a behavioural change, i.e. namespaced attributes id and classes would not be considered after this change.

(Honestly speaking, I am not sure whether there is a sensible namespace for these? html? So maybe a second lookup for QualName::new(None, ns!(html), "id") would suffice?)

((I added once_cell as a direct dependency but it already was part of the dependency closure.))

@adamreichold
Copy link
Member Author

This does come with an API change (no more public fields id and classes)

Of course, this could be changed as well, i.e. we could provide a getter for classes which yields a &HashSet<LocalName> and we could also keep the same exhaustive scan for both id and classes while still doing that only lazily. I am just not sure whether this is worth the additional costs.

@cfvescovo
Copy link
Member

This is a good idea. Before merging we need to discuss about the breaking changes though. This could be included in the next major release. cc @teymour-aldridge

@adamreichold
Copy link
Member Author

This is a good idea. Before merging we need to discuss about the breaking changes though.

I would also like to note that I think the behaviour changes (id with a namespace is ignored) are even more important to discuss than the purely structural API changes.

@adamreichold
Copy link
Member Author

This is a good idea. Before merging we need to discuss about the breaking changes though.

I would also like to note that I think the behaviour changes (id with a namespace is ignored) are even more important to discuss than the purely structural API changes.

I rebased this and changed it so that the ID is stored lazily as well and hence the semantics are completely unchanged compared to the master branch. Just the storage resp. extraction is handled differently.

@cfvescovo
Copy link
Member

Since there are no breaking changes, we can merge this right now. @adamreichold do you want to add something else?

@adamreichold
Copy link
Member Author

Since there are no breaking changes, we can merge this right now.

Note that these are semantically equivalent but still technically breaking as both id and classes cannot be accessed via public field after this, but only via the respective methods. So merging this does imply a version bump.

@adamreichold do you want to add something else?

No I don't plan to add anything else.

@cfvescovo
Copy link
Member

Since there are no breaking changes, we can merge this right now.

Note that these are semantically equivalent but still technically breaking as both id and classes cannot be accessed via public field after this, but only via the respective methods. So merging this does imply a version bump.

Yeah, I meant that there are no substantial changes in usage. Obviously a version bump will be necessary. (0.16.0)

@cfvescovo cfvescovo merged commit 8a3723f into rust-scraper:master Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants