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

Remove the Repeated API in 6.0.0 #1296

Closed
thomaslevesque opened this issue Feb 2, 2018 · 6 comments · Fixed by #1678
Closed

Remove the Repeated API in 6.0.0 #1296

thomaslevesque opened this issue Feb 2, 2018 · 6 comments · Fixed by #1678

Comments

@thomaslevesque
Copy link
Member

Related to #1292, #808, #1295

@thomaslevesque
Copy link
Member Author

What do we do about the FakeItEasy0006 diagnostic?

  1. Remove it.

    Pro: less code, yeah!
    Con: if someone uses FakeItEasy < 6.0.0 and FakeItEasy.Analyzer.* >= 6.0.0, they won't get the warnings (well, they'll have the compiler warnings for using obsolete members, but no code fixes).

  2. Keep it

    Pro: warnings will show even when using latest analyzer and older library
    Con: we can't test it anymore, because the testing code wouldn't compile (since it references the current version of FakeItEasy).

Personally, I think option 1 is fine. There's no good reason to have mismatched versions of the library and analyzer, since they're versioned together. We can probably consider this an unsupported scenario.

@blairconrad what do you think?

@blairconrad
Copy link
Member

Ugh. Let me think out loud about when keeping the diagnostic would help

  1. Someone uses Repeated, and either never used FakeItEasy 5+, or ignored the obsolete warnings.
  2. Presumably they also haven't been using an analyzer of version 4.5.1+. Or if so, they ignored the warning.
  3. They upgrade to FakeItEasy 6.0.0, and compilation fails.
  4. They have chosen this time to pick up a shiny new analyzer.

As much as it pains me to say this, because I don't want to take away the diagnostic when it's arguably most useful, I do not see there being a large audience.
And there's a workaround - quick install an earlier analyzer, convert from Repeated, and then upgrade the world.

Initially I thought about leaving the diagnostic there untested, and then thought "we can actually test even if the compilation fails", but the diagnostic is written to make use of information from a successful compilation (do I understand that correctly? ConvertedType seems to be an error when we are compiling the test code against a FIE that lacks Repeated). And I do not have the appetite to rewrite the diagnostic to make guesses about what this "Repeated" symbol means and how to convert to the new form, even though I bet it's achievable just by considering the source as text nodes.

So... remove it?

@thomaslevesque
Copy link
Member Author

but the diagnostic is written to make use of information from a successful compilation

That's correct. But even if we were able to test the diagnostic on an unsuccessful compilation, it wouldn't help in this case, because it couldn't resolve Repeated to FakeItEasy.Repeated (since it doesn't exist)

@thomaslevesque
Copy link
Member Author

So... remove it?

That was my preference as well

@blairconrad
Copy link
Member

That's correct. But even if we were able to test the diagnostic on an unsuccessful compilation, it wouldn't help in this case, because it couldn't resolve Repeated to FakeItEasy.Repeated (since it doesn't exist)

Ah. I thought maybe the diagnostic could be rewritten to take it on faith that a chain starting with "Repeated" in the appropriate context actually referred to a FakeItEasy Repeated and we could manipulate the tree anyhow. It would introduce the (very very low) risk of false positives, of course.
But even if it is possible, like I say, I have no appetite for putting that much effort in.

@afakebot
Copy link

This change has been released as part of FakeItEasy 6.0.0-beta.1.

This was referenced Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants