Skip to content

Fix the return type of TypeAssertions.HaveAccessModifier and NotHaveAccessModifier#1159

Merged
jnyrup merged 4 commits intofluentassertions:developfrom
adrianiftode:fix-type-assertions-return-type-for-have-access-modifier
Feb 21, 2020
Merged

Fix the return type of TypeAssertions.HaveAccessModifier and NotHaveAccessModifier#1159
jnyrup merged 4 commits intofluentassertions:developfrom
adrianiftode:fix-type-assertions-return-type-for-have-access-modifier

Conversation

@adrianiftode
Copy link
Copy Markdown
Contributor

@adrianiftode adrianiftode commented Oct 8, 2019 •
edited
Loading

#1154

The changes don't include a unit test, because I didn't see a common practice in the code base to test that the AndConstraint is closed with the correct type. The only test I've seen that checks for this particular situation is When_using_StringCollectionAssertions_the_AndConstraint_should_have_the_correct_type. I could add something similar.

  • If the contribution affects the documentation, please include your changes to documentation.md in this pull request so the documentation will appear on the website.

There are no references in docs to these two assertions

TypeAssertions HaveAccessModifier and not NotHaveAccessModifier
8fc4b9c 
return AndConstraint with TypeAssertions instead one with Type

fluentassertions#1154
@adrianiftode adrianiftode changed the title TypeAssertions HaveAccessModifier and not NotHaveAccessModifier return AndConstraint with TypeAssertions instead one with Type Fix the return type of TypeAssertions.HaveAccessModifier and NotHaveAccessModifier Oct 8, 2019
@dennisdoomen dennisdoomen requested a review from jnyrup October 9, 2019 07:35
@dennisdoomen
Copy link
Copy Markdown
Member

dennisdoomen commented Oct 9, 2019

I wondering whether we should treat this as a binary compatibility issue.

@adrianiftode
Copy link
Copy Markdown
Contributor Author

adrianiftode commented Oct 9, 2019

It will be an issue, because the return type is changed and it will throw a MethodNotFoundException. The clients will need to recompile.

So for let's say if the current usage is something like:

type.Should().HaveAccessModifier(CSharpAccessModifier.Internal).And.Should().BeAbstract();

After this change the required step would be:

type.Should().HaveAccessModifier(CSharpAccessModifier.Internal).And.BeAbstract();

If the AndConstraint is not used, then the clients don't have to change the source code, just to recompile.

@dennisdoomen
Copy link
Copy Markdown
Member

dennisdoomen commented Oct 10, 2019

Yeah, so technically this would be a breaking change. But then again, how likely is this going to be a realistic problem. @jnyrup what do you think?

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Oct 10, 2019

I'm maybe being over-conservative, but I don't think the benefits are worth the breaking change.
I fully agree that this should be fixed in next major update.

@jnyrup jnyrup added this to the 6.0 milestone Nov 18, 2019
@dennisdoomen dennisdoomen changed the base branch from master to develop January 4, 2020 08:18
@dennisdoomen
Copy link
Copy Markdown
Member

dennisdoomen commented Jan 4, 2020

Can we continue with this one now that we're accepting potentially breaking changes again?

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Jan 4, 2020

Let's continue with this!

@adrianiftode Note that since #1217 any API change must be changed in the relevant approved.txt files.
When you run the tests locally, received.txt files are generated in Tests/Approval.Tests/ApprovedApi/FluentAssertions with the API changes.
Copy these into approved.txt files to make the approval tests pass.

@jnyrup jnyrup linked an issue Feb 14, 2020 that may be closed by this pull request
TypeAssertions.HaveAccessModifier and TypeAssertions.NotHaveAccessModifier return AndConstraint<Type> instead of AndConstraint<TypeAssertions> #1154
Closed
@adrianiftode adrianiftode force-pushed the fix-type-assertions-return-type-for-have-access-modifier branch from 7299423 to c01d8cc Compare February 21, 2020 13:37
@adrianiftode
Copy link
Copy Markdown
Contributor Author

adrianiftode commented Feb 21, 2020

@jnyrup I've run the tests as instructed.

When I first time run the tests my editor added an extra line at the end of the files and the tests failed. I wonder if these lines could be relaxed/ignored while asserting it.

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Feb 21, 2020

Sorry, the flow has been improved since I suggested to "copy over the changes".
It should not be necessary to involve an IDE.
In the root folder of the project we now have AcceptApiChanges.{sh,ps1} to do the changes.
Would that avoid the problems you had?

@jnyrup jnyrup requested a review from dennisdoomen February 21, 2020 14:03
@adrianiftode
Copy link
Copy Markdown
Contributor Author

adrianiftode commented Feb 21, 2020

I just tried it and looks fine.

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Feb 21, 2020

While I remember it...
Could I ask you to add a line to docs/_pages/releases.md under "Breaking Changes" with the changes done?

@jnyrup jnyrup merged commit 4262601 into fluentassertions:develop Feb 21, 2020
@adrianiftode adrianiftode deleted the fix-type-assertions-return-type-for-have-access-modifier branch February 21, 2020 14:47
@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Feb 21, 2020

@adrianiftode Thanks again for reporting and contributing a fix for this issue.

@adrianiftode
Copy link
Copy Markdown
Contributor Author

adrianiftode commented Feb 22, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dennisdoomen dennisdoomen dennisdoomen approved these changes

@jnyrup jnyrup jnyrup approved these changes

Assignees

No one assigned

Labels

breaking change

Projects

None yet

Milestone

6.0

Development

Successfully merging this pull request may close these issues.

TypeAssertions.HaveAccessModifier and TypeAssertions.NotHaveAccessModifier return AndConstraint<Type> instead of AndConstraint<TypeAssertions>

3 participants

@adrianiftode @dennisdoomen @jnyrup