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

Funcky.Xunit - Analyzer for Assert.Equal with FunctionalAssert.Ok/Some/Error/etc. in the actual parameter #749

Open
Mafii opened this issue Sep 18, 2023 · 5 comments
Labels
area: Analyzer enhancement New feature or request

Comments

@Mafii
Copy link
Member

Mafii commented Sep 18, 2023

Rudimentary example without context:

image

Can be simplified to

image

@Mafii Mafii added enhancement New feature or request area: Analyzer labels Sep 18, 2023
@bash
Copy link
Member

bash commented Sep 18, 2023

This would be the first analyzer that analyzes Funcky.Xunit 🚀

It should of course come with an accompanying code fix.

So in summary, this analyzer would suggest the following changes to users:

  • Rewrite Assert.Equal(expected, FunctionalAssert.Some(actual)) to FunctionalAssert.Some(expected, actual)
  • There is no overload for FunctionalAssert.None
  • Rewrite Assert.Equal(expected, FunctionalAssert.Ok(actual)) to FunctionalAssert.Ok(expected, actual)
  • Rewrite Assert.Equal(expected, FunctionalAssert.Error(actual)) to FunctionalAssert.Error(expected, actual) This overload does not exist.
  • Rewrite Assert.Equal(expected, FunctionalAssert.Left(actual)) to FunctionalAssert.Left(expected, actual)
  • Rewrite Assert.Equal(expected, FunctionalAssert.Right(actual)) to FunctionalAssert.Right(expected, actual)

Instead of having a hardcoded list of FunctionalAssert methods to check, I suggest adding an attribute (naming idea off the top of my head [HasAssertEqualOverload]).

If you want to implement this and need some guidance to get started, you can give me a shout on Discord :)

@janhohenheim
Copy link
Contributor

@Mafii any updates?

@Mafii
Copy link
Member Author

Mafii commented Sep 20, 2023

Sounds good! I don't know if I want to do this, or what the timeline is, but it would be a useful feature, so let's see if and when it happens :)

@bash
Copy link
Member

bash commented Sep 21, 2023

We need to be very careful for which cases we emit a warning.

There are two things to consider:

  • Assert.Equal has overloads which take more than two parameters, such as those that take a custom comparer or the precision for floats.
    → The solution is simple: Just don't emit a warning for these overloads.
  • Assert.Equal<T> specializes for some types, such as IEnumerable. Our overloads to not account for this as we call Assert.Equal<Option<T>> losing this specialization.
    → This is a bit trickier: Do we want to account for these specialization in our analyzer or should we change the implementation of FunctionalAssert?

@Mafii
Copy link
Member Author

Mafii commented Sep 21, 2023

That's tricky! Both would be fine - adding specializations for FunctionalAssert (that's a lot of effort) or not emitting the analyzer warning. I think the second one is easier, as it can be applied partially instead of having to cover all cases immediately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Analyzer enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants