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

feat: add html element extractor #229

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

a2937
Copy link
Member

@a2937 a2937 commented Aug 16, 2024

Checklist:

Closes #XXXXX

A variation of this particular regex was used in PRs like freeCodeCamp/freeCodeCamp#55547. I expect this kind of thing to come up a little bit more often in the future which is why I'm adding it to your curriculum helpers. I've also generalized this in an attempt to increase the usefulness. Though without that extra parameter present it will just extract head elements.

@a2937 a2937 requested a review from a team as a code owner August 16, 2024 17:39
@ShaunSHamilton
Copy link
Member

We can definitely add this, but I disagree with this expectation:

I expect this kind of thing to come up a little bit more often in the future which is why I'm adding it to your curriculum helpers.

Ideally, Regex should NOT be used at all for the tests. In that one case, it is forcibly used, because the browser changes the html in the preview.

@a2937
Copy link
Member Author

a2937 commented Aug 19, 2024

Yeah ideally it shouldn't.

Co-authored-by: Naomi the Technomancer <[email protected]>
@a2937
Copy link
Member Author

a2937 commented Aug 19, 2024

I also hopefully will find a way to convert this from Regex into something that uses string manipulation methods.

@ojeytonwilliams
Copy link
Contributor

Hey @a2937 @ShaunSHamilton is this still something we want? Shaun, I know you had concerns.

@a2937
Copy link
Member Author

a2937 commented Mar 10, 2025

I can update the description and make the merge changes if this is something that seems useful.

@ShaunSHamilton
Copy link
Member

I think this can be useful. We just need the suggested changes and conflicts.

Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

This would be confusing if we forget it defaults to head - there is no logical reason to default.

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.

4 participants