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.

Sign up for GitHub

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

Jump to bottom

html: add parse_html #436

Merged
merged 1 commit into from
Jul 14, 2023
Merged

html: add parse_html #436

merged 1 commit into from
Jul 14, 2023

Conversation

fscherf
Copy link
Member

@fscherf fscherf commented Jul 7, 2023 •
edited
Loading

This PR renames lona.html.parsing.html_string_to_node_list to lona.html.parse_html to make it official API that will replace HTML parsing in lona.html.HTML and lona.html.HTML2 (#268)

@fscherf fscherf mentioned this pull request Jul 7, 2023
Open
@SmithChart
Copy link
Contributor

How does renaming make this an official API?
We probably want to add it to the documentation though :-)

Otherwise LGTM.

@fscherf
Copy link
Member Author

fscherf commented Jul 10, 2023

How does renaming make this an official API?

lona.html.parsing.html_string_to_node_list was never meant to be documented, because the name is so clunky. Giving it a cleaner name and putting it into the documentation makes it "official" :)

@fscherf
html: add parse_html
d2e6824 
Signed-off-by: Florian Scherf <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.03 ⚠️

Comparison is base (d0b50b9) 71.30% compared to head (d2e6824) 71.27%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   71.30%   71.27%   -0.03%     
==========================================
  Files          83       83              
  Lines        5697     5699       +2     
  Branches     1226     1227       +1     
==========================================
  Hits         4062     4062              
- Misses       1345     1346       +1     
- Partials      290      291       +1
Impacted Files Coverage Δ
lona/html/parsing.py 87.50% <71.42%> (-1.17%) ⬇️
lona/html/__init__.py 100.00% <100.00%> (ø)
lona/html/widgets.py 62.50% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fscherf fscherf marked this pull request as ready for review July 10, 2023 07:53
@fscherf fscherf requested a review from SmithChart July 10, 2023 07:54
@fscherf
Copy link
Member Author

fscherf commented Jul 13, 2023

@SmithChart: I updated the documentation and all tests are green. I think in this state this is mergeable and closes #268

SmithChart
SmithChart approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@SmithChart SmithChart left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good.

@fscherf
Copy link
Member Author

fscherf commented Jul 14, 2023

@SmithChart: \o/ Thanks for your review and your patience

@fscherf fscherf merged commit 363b389 into master Jul 14, 2023
@fscherf fscherf deleted the fscherf/add-parse-html branch July 14, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SmithChart SmithChart SmithChart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fscherf @SmithChart @codecov-commenter