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

Add overload to HaveElement() to be able to assert on occurrences for XDocument and XElement #1880

Merged
merged 47 commits into from Apr 23, 2022

Conversation

ITaluone
Copy link
Contributor

@ITaluone ITaluone commented Apr 5, 2022

This ref. #1681 and #1684
Closes #1681

Try to finish the work of @ABergBN

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
  • If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
  • If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
  • If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

@coveralls
Copy link

coveralls commented Apr 5, 2022

Pull Request Test Coverage Report for Build 2205695974

  • 56 of 56 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 95.607%

Totals Coverage Status
Change from base Build 2197485723: 0.02%
Covered Lines: 8502
Relevant Lines: 8769

💛 - Coveralls

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Apr 5, 2022

Maybe using the OccurrenceConstaints for asserting on the expected count?

@ITaluone ITaluone changed the title Implement HaveElementCount() and HaveSingleElement() for XDocument Add overload to HaveElement() to be able to assert on occurrences and add HaveSingleElement() for XDocument Apr 6, 2022
@ITaluone ITaluone marked this pull request as ready for review Apr 6, 2022
@jnyrup
Copy link
Member

jnyrup commented Apr 7, 2022

It's a bit difficult to review, when the APIs keep changing 😉
You can always mark a PR as draft.

We are trying to formalize the process of changes a bit, such that larger changes are first discussed in an issue before opening a PR.
Typos and alike are always welcome and don't need to go through an issue.

We haven't fully set on the process, but these links should provide a feeling of it, notably excluding the amount of manpower we can allocate.
https://devblogs.microsoft.com/dotnet/api-review-process-for-net-core/
https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Apr 7, 2022

This PR just finishes an already discussed feature.
The only difference by now is to use the OccurrenceConstraint, which makes more sense IMO

ahh.. and btw. This state is my „final“ change 🙃

Copy link
Member

@dennisdoomen dennisdoomen left a comment

📝 I leave a code review comment only once and assume that similar code needs similar improvements.
🤔 For consistency, we also need to add those new members to XElementAssertions.

Src/FluentAssertions/Xml/XDocumentAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XDocumentAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XDocumentAssertions.cs Outdated Show resolved Hide resolved
Copy link
Member

@jnyrup jnyrup left a comment

As it seems you like to learn about our process.
You're not "just" continuing another PR when you change the API. ℹ️

The main reason I asked about discussing the API again, was because I like the usage of OccurrenceConstraint and had ideas to improve it even more 🚀

@ITaluone ITaluone force-pushed the issue-1681 branch 3 times, most recently from 401ec08 to 5b1ae03 Compare Apr 8, 2022
@ITaluone ITaluone changed the title Add overload to HaveElement() to be able to assert on occurrences and add HaveSingleElement() for XDocument Add overload to HaveElement() to be able to assert on occurrences and add HaveSingleElement() for XDocument and XElement Apr 8, 2022
@ITaluone
Copy link
Contributor Author

ITaluone commented Apr 8, 2022

@dennisdoomen What do you think: Should I remove the HaveSingleElement() method?

@dennisdoomen
Copy link
Member

dennisdoomen commented Apr 9, 2022

HaveSingleElement() reads less "fluent" than HaveElement("x", Exactly.Once());

I have to admit I agree. The only case where it would read fluent if there was a parameterless HaveSingleElement.

@jnyrup
Copy link
Member

jnyrup commented Apr 16, 2022

Is the the API change we can agree on?

class XDocumentAssertions
{
+    AndWhichConstraint<XDocumentAssertions, IEnumerable<XElement>> HaveElement(string expected, OccurrenceConstraint occurrenceConstraint, string because = "", params object[] becauseArgs)
+    AndWhichConstraint<XDocumentAssertions, IEnumerable<XElement>> HaveElement(XName expected, OccurrenceConstraint occurrenceConstraint, string because = "", params object[] becauseArgs)
}

class XElementAssertions
{
+    AndWhichConstraint<XElementAssertions, IEnumerable<XElement>> HaveElement(string expected, OccurrenceConstraint occurrenceConstraint, string because = "", params object[] becauseArgs)
+    AndWhichConstraint<XElementAssertions, IEnumerable<XElement>> HaveElement(XName expected, OccurrenceConstraint occurrenceConstraint, string because = "", params object[] becauseArgs)
}

@dennisdoomen
Copy link
Member

dennisdoomen commented Apr 16, 2022

Is the the API change we can agree on?

Yep

@dennisdoomen dennisdoomen added the api-approved label Apr 16, 2022
Copy link
Contributor

@IT-VBFK IT-VBFK left a comment

Move release notes to the appropriate section

and/or rebase to current develop

docs/_pages/xml.md Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XDocumentAssertions.cs Outdated Show resolved Hide resolved
@ITaluone ITaluone requested a review from jnyrup Apr 20, 2022
@ITaluone ITaluone requested a review from jnyrup Apr 22, 2022
@jnyrup jnyrup added the feature label Apr 23, 2022
jnyrup
jnyrup approved these changes Apr 23, 2022
@jnyrup jnyrup merged commit 68f41b9 into fluentassertions:develop Apr 23, 2022
1 check passed
@ITaluone ITaluone deleted the issue-1681 branch Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants