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

Provide content options for HTML attribute extractor (fixes #56) #57

Merged

Conversation

arm1n
Copy link
Contributor

@arm1n arm1n commented Oct 19, 2022

Hi Lukas,

this PR introduces the possibility to use content options for HtmlExtractors.elementAttribute the same way as for HtmlExtractors.elementContent. As usages including default values should very likely be the same I had to move some code into utils for reusability. Besides, all attributes being used get the same content options applied, which was added in the primitive element extractor. Tests should cover these scenarios.

Looking forward to your feedback, thanks!

Copy link
Owner

@lukasgeiter lukasgeiter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Armin, thanks for your work! It's looking pretty good already. Just a few changes and we'll get this merged 🙂

src/html/utils.ts Outdated Show resolved Hide resolved
src/utils/content.ts Show resolved Hide resolved
@arm1n arm1n force-pushed the feat-attribute-content-options branch from 09376bd to 6533293 Compare October 24, 2022 09:26
@arm1n
Copy link
Contributor Author

arm1n commented Oct 24, 2022

Thanks for your feedback and code review. I've added the changes and would ask you to have one more look, thanks Lukas!

yarn.lock Outdated Show resolved Hide resolved
src/html/utils.ts Outdated Show resolved Hide resolved
src/html/extractors/factories/elementAttribute.ts Outdated Show resolved Hide resolved
@arm1n arm1n force-pushed the feat-attribute-content-options branch from 6533293 to b4a07de Compare October 25, 2022 14:32
@arm1n
Copy link
Contributor Author

arm1n commented Oct 25, 2022

Reiterated the PR according to your feedback, thanks for taking the time to look into it.

@lukasgeiter lukasgeiter merged commit f1e7fbf into lukasgeiter:master Oct 25, 2022
@arm1n arm1n deleted the feat-attribute-content-options branch October 26, 2022 06:52
@arm1n
Copy link
Contributor Author

arm1n commented Oct 26, 2022

Thanks for merging Lukas, we should document the content options also on the corresponding wiki page.

@lukasgeiter
Copy link
Owner

Of course! I'll update the wiki publish a release soon

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.

None yet

2 participants