Context finder for strings that start with an html5 tag #220

egil · 2020-09-27T21:15:54Z

Pull request description

Fixes #198. This is an attempt to implement a automatic context detection if the string to be parsed by BunitHtmlParser starts with a HTML tag.

PR meta checklist

Pull request is targeting at DEV branch.
Pull request is linked to all related issues, if any.
I have read the CONTRIBUTING.md document.

Content checklist

My code follows the code style of this project and AspNetCore coding guidelines.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
I have updated the appropriate sub section in the CHANGELOG.md.
I have added, updated or removed tests to according to my changes.
- All tests passed.

* componentparametercollection * Support for passing multiple template and render fragments * Automated dotnet-format update * cascading values * Finished component parameter collection, closed #142, updated tests to component parameter factory * Fixed null warnings * Automated dotnet-format update * Fixes for CodeQL warnings * Added ChildContent and RenderFragment tests * Added support for passing template fragments * Removed ComponentParameterBuilder, added support for unmatched and cascading values * Switched to C# 9 compile * Automated dotnet-format update * unnamed cascading value with add * Removed .net move hack from workflows * Fix for null errors * Automated dotnet-format update * Added extra factory method and moved ComponentParameter out into Bunit namespace * Updated docs with new parameter passing logic * Automated dotnet-format update * Updated changelog Co-authored-by: Github Actions <[email protected]>

codecov · 2020-09-27T21:22:25Z

Codecov Report

❗ No coverage uploaded for pull request base (dev@fafa45c). Click here to learn what that means.
The diff coverage is 81.45%.

@@          Coverage Diff           @@
##             dev     #220   +/-   ##
======================================
  Coverage       ?   79.17%           
======================================
  Files          ?      103           
  Lines          ?     3011           
  Branches       ?      394           
======================================
  Hits           ?     2384           
  Misses         ?      498           
  Partials       ?      129

Impacted Files	Coverage Δ
src/bunit.core/ComponentParameterCollection.cs	`80.35% <ø> (ø)`
.../bunit.core/ComponentParameterCollectionBuilder.cs	`81.25% <ø> (ø)`
src/bunit.core/ComponentParameterFactory.cs	`100.00% <ø> (ø)`
...re/Extensions/RenderedComponentRenderExtensions.cs	`74.35% <ø> (ø)`
....web/Asserting/ShouldBeAdditionAssertExtensions.cs	`0.00% <0.00%> (ø)`
...t.web/Asserting/ShouldBeRemovalAssertExtensions.cs	`0.00% <0.00%> (ø)`
...eb/Asserting/ShouldBeTextChangeAssertExtensions.cs	`0.00% <0.00%> (ø)`
...it.web/Extensions/Internal/AngleSharpExtensions.cs	`33.33% <0.00%> (ø)`
src/bunit.web/TestContext.cs	`100.00% <ø> (ø)`
...nit.web/Asserting/MarkupMatchesAssertExtensions.cs	`12.50% <25.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fafa45c...a17f120. Read the comment docs.

FlorianRappl

I think this looks good.

As suggested, I would only create the document + elements locally and separate the parser instance from the document instance. Also I would go for the async variant. However, as it is it should also just work fine.

src/bunit.web/Rendering/BunitHtmlParser.cs

egil · 2020-09-28T06:37:54Z

Huge thanks for the review.

In all the cases where this is used in bUnit isnt async, so from that point of view it does not make sense to make the parse method async.

I am not sure I follow your suggestion, can you share a link to some docs are a snippet that shows what you are thinking of?

Last question. I would like to support parsing html, head, and body start tags, but haven't figured out how to do so with the contexts. Any suggestions?

FlorianRappl · 2020-09-28T07:02:11Z

I am not sure I follow your suggestion, can you share a link to some docs are a snippet that shows what you are thinking of?

I am not sure which suggestion you mean. Locality means avoid having members, but just local references, e.g.,

public INodeList Parse(string markup)
{
  var document = ...
  var body = ...
  return ParseInto(document, body, ..., markup); // just an example call, you could also make a local function and capture the variables above
}

Last question. I would like to support parsing html, head, and body start tags, but haven't figured out how to do so with the contexts.

For the given three tags I would not use a fragment, but rather full parsing. These are not fragments but (parts of) full documents.

egil · 2020-09-28T07:16:12Z

Ahh yes, that's what I meant. Is it purely esthetics that you prefer locality or is there also some internal AngleSharp reason not to reuse the "contexts"?

Update: saw your other comment on the code, so I guess the answer is yes to the last part.

FlorianRappl · 2020-09-28T07:29:29Z

Update: saw your other comment on the code, so I guess the answer is yes to the last part.

If yes means "has functional consequences" then yes. You should always prefer locals over members. If that parser is reused you will be in deep trouble as there are already members on them.

egil · 2020-09-28T09:03:02Z

Update: saw your other comment on the code, so I guess the answer is yes to the last part.

If yes means "has functional consequences" then yes. You should always prefer locals over members. If that parser is reused you will be in deep trouble as there are already members on them.

How so? The current version in main does the same thing, and that is typically scoped to a test, such that all parsing needs within a single test share the document.

What kind of problems should this be causing?

FlorianRappl · 2020-09-28T09:16:22Z

The current version in main does the same thing, and that is typically scoped to a test, such that all parsing needs within a single test share the document.

Again, if you reuse the parser you reuse the document you end up with unexpected results. This can cause all kinds of problems - usually you want that locality (it does not only help the GC, but it implies a more advanced development paradigm that avoids side-effects).

I mean this discussion quite derailed right now. So I'd like to close it. If you want to have members - sure go ahead. All I am writing is that keeping local things local does help. It also helps thinking just in the context of one class (I don't want to look at uses of this class to understand if having members is acceptable or not - just avoiding it would be the way to go to prevent unnecessary cognitive load in the first place).

egil · 2020-09-28T09:54:28Z

I mean this discussion quite derailed right now...

That's fair. I am not pro one or the other, but I am trying to understand how AngleSharp works and how to use it correct in this regard.

egil · 2020-09-29T19:58:34Z

@FlorianRappl I did as you suggested and stopped reusing the context elements. Also have support for html, head and body now. Does this look alright?

(code is a bit messy but I can refactor it later when my flu passes).

sonarqubecloud · 2020-10-02T09:12:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

egil and others added 5 commits September 15, 2020 22:15

reset changelog with unreleased section

aac655e

Cleanup using and resorted them

4f35197

Updated version to beta-11

b612bd0

Context finder for strings that start with an html5 tag

9ea7db3

egil linked an issue Sep 27, 2020 that may be closed by this pull request

bUnit's HtmlParser should detect markup context to avoid surprising users #198

Closed

Fix for build error

e3c7712

FlorianRappl approved these changes Sep 28, 2020

View reviewed changes

src/bunit.web/Rendering/BunitHtmlParser.cs Outdated Show resolved Hide resolved

src/bunit.web/Rendering/BunitHtmlParser.cs Show resolved Hide resolved

Added support for HTML, HEAD, and BODY, stopped reusing element contexts

d861c75

egil force-pushed the dev branch from b612bd0 to fafa45c Compare October 1, 2020 16:23

egil added 2 commits October 2, 2020 09:07

Merge branch 'dev' into bug/198-htmlparser-context

6524996

Cleanup usings

a17f120

egil merged commit 2611704 into dev Oct 2, 2020

egil deleted the bug/198-htmlparser-context branch October 2, 2020 09:14

egil mentioned this pull request Oct 7, 2020

feature/227 assertionmethod #229

Merged

Uh oh!

Context finder for strings that start with an html5 tag #220

Context finder for strings that start with an html5 tag #220

Uh oh!

Conversation

egil commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request description

PR meta checklist

Content checklist

Uh oh!

codecov bot commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

FlorianRappl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

egil commented Sep 28, 2020

Uh oh!

FlorianRappl commented Sep 28, 2020

Uh oh!

egil commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FlorianRappl commented Sep 28, 2020

Uh oh!

egil commented Sep 28, 2020

Uh oh!

FlorianRappl commented Sep 28, 2020

Uh oh!

egil commented Sep 28, 2020

Uh oh!

egil commented Sep 29, 2020

Uh oh!

sonarqubecloud bot commented Oct 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

egil commented Sep 27, 2020 •

edited

Loading

codecov bot commented Sep 27, 2020 •

edited

Loading

egil commented Sep 28, 2020 •

edited

Loading