Skip to content

Refactor the IntToBoolConverter to use the BaseConverter#327

Merged
TheCodeTraveler merged 10 commits intomainfrom
feature/int-to-bool-converter-to-use-base
Mar 20, 2022
Merged

Refactor the IntToBoolConverter to use the BaseConverter#327
TheCodeTraveler merged 10 commits intomainfrom
feature/int-to-bool-converter-to-use-base

Conversation

@bijington
Copy link
Copy Markdown
Contributor

@bijington bijington commented Mar 16, 2022

Description of Change

Refactors the IntToBoolConverter to use the BaseConverter.

Linked Issues

n/a

PR Checklist

Additional information

@bijington bijington changed the title Refactor to using common base class Refactor the IntToBoolConverter to use the BaseConverter Mar 16, 2022
Comment thread src/CommunityToolkit.Maui/Converters/IntToBoolConverter.shared.cs Outdated
@TheCodeTraveler
Copy link
Copy Markdown
Collaborator

Adding the blocked tag until #325 is merged

Copy link
Copy Markdown
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

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

Thanks Shaun!

Could you also fix the Unit Test Method names? It looks like the current method names are leftover from a copy/paste of IndexToArrayConverter's tests:

  • IntToBoolConverter_Tests.IndexToArrayConverter -> IntToBoolConverter_Tests.IntToBoolConverter
  • IntToBoolConverter_Tests.IndexToArrayConverterBack -> IntToBoolConverter_Tests.IntToBoolConverterBack

Comment on lines +19 to +20
Assert.Equal(result, expectedResult);
Assert.Equal(typedResult, expectedResult);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's swap these Assert.Equal parameters around.

It's kinda trivial, but when the Assert fails, it'll print "...Expected: [first parameter] Actual: [second parameter]" which can be confusing if the parameters are reversed

Suggested change
Assert.Equal(result, expectedResult);
Assert.Equal(typedResult, expectedResult);
Assert.Equal(expectedResult, result);
Assert.Equal(expectedResult, typedResult);

Comment on lines +33 to +34
Assert.Equal(result, expectedResult);
Assert.Equal(typedResult, expectedResult);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's swap these Assert.Equal parameters around.

It's kinda trivial, but when the Assert fails, it'll print "...Expected: [first parameter] Actual: [second parameter]" which can be confusing if the parameters are reversed.

Suggested change
Assert.Equal(result, expectedResult);
Assert.Equal(typedResult, expectedResult);
Assert.Equal(expectedResult, result);
Assert.Equal(expectedResult, typedResult);

[InlineData("")]
[InlineData(null)]
public void InValidConverterValuesThrowArgumenException(object value)
public void InValidConverterValuesThrowArgumentException(object value)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's fix this small typo from a previous PR

Suggested change
public void InValidConverterValuesThrowArgumentException(object value)
public void InvalidConverterValuesThrowArgumentException(object value)

[InlineData("")]
[InlineData(null)]
public void InValidConverterBackValuesThrowArgumenException(object value)
public void InValidConverterBackValuesThrowArgumentException(object value)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's fix this small typo from a previous PR

Suggested change
public void InValidConverterBackValuesThrowArgumentException(object value)
public void InvalidConverterBackValuesThrowArgumentException(object value)

@TheCodeTraveler TheCodeTraveler enabled auto-merge (squash) March 20, 2022 19:30
@bijington
Copy link
Copy Markdown
Contributor Author

@brminnick rhanks for the fix. I haven't removed the extensions around asserting the Excoetional conditions. What are your thoughts?

@TheCodeTraveler
Copy link
Copy Markdown
Collaborator

Hey Shaun! I went ahead and pulled out the converter assert extension methods. I think it's best to keep the Unit Tests as simple/dumb as possible to make it easier for us to understand why unit tests are failing when writing future PRs.

For example, when Vincent Hoogendoorn wrote the unit tests for the Markup library, he made them very complex and now it's tough to understand what a Unit Test is doing and, worse, to understand why a Unit Test is failing: https://github.com/CommunityToolkit/Maui.Markup/blob/main/src/CommunityToolkit.Maui.Markup.UnitTests/BindableObjectExtensionsTests.cs

@bijington
Copy link
Copy Markdown
Contributor Author

No problem, I get that argument and missed you had removed them. I wanted to make sure before this went in 😃

@TheCodeTraveler TheCodeTraveler merged commit 37be25f into main Mar 20, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/int-to-bool-converter-to-use-base branch March 20, 2022 20:02
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants