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

Support .NET 8 #2465

Open
vbreuss opened this issue Nov 15, 2023 · 15 comments
Open

Support .NET 8 #2465

vbreuss opened this issue Nov 15, 2023 · 15 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation feature

Comments

@vbreuss
Copy link
Contributor

vbreuss commented Nov 15, 2023

Background and motivation

On Nov. 14th .NET 8 was released. As this is a major version, I suggest that fluentassertions should also support .NET 8 as Target Framework.

Alternative Concerns

No response

Are you willing help with a pull-request?

Yes, please assign this issue to me.

@ITaluone
Copy link
Contributor

See discussion here: #2251

And somewhat here: #1779 (further down)

@vbreuss
Copy link
Contributor Author

vbreuss commented Nov 15, 2023

I don't think it is necessary to explicitely target .NET 8 in fluentassertions.csproj, but we should update the test suite to run on .NET 8 to make sure, that it can also be used in .NET 8.

@vbreuss vbreuss mentioned this issue Nov 15, 2023
7 tasks
@vbreuss
Copy link
Contributor Author

vbreuss commented Nov 15, 2023

While testing it locally I stumbled upon one issue:
.NET 8 introduced an own ITimer interface. This results in a "CS0104" warning: 'ITimer' is an ambiguous reference between 'FluentAssertions.Common.ITimer' and 'System.Threading.ITimer'.

In the FakeClock class I added these lines to fix the issue, but I fear this could also affect all users:

#if NET8_0_OR_GREATER
using ITimer = FluentAssertions.Common.ITimer;
#endif

@dennisdoomen dennisdoomen added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed triage labels Nov 15, 2023
@dennisdoomen
Copy link
Member

Until we need to support specific features, we don't need to target .NET 8 for the main project. However, adding .NET 8 as a target for the tests is indeed useful. This probably also require bumping the SDK in the global.json.

Next to that, maybe this mean we should either rename our ITimer to something else, or adopt the new interface (and provide a polyfill of some sort, e.g. through PolySharp).

@vbreuss
Copy link
Contributor Author

vbreuss commented Nov 15, 2023

Next to that, maybe this mean we should either rename our ITimer to something else, or adopt the new interface (and provide a polyfill of some sort, e.g. through PolySharp).

The two interfaces are not really compatible:

  • Our ITimer consists of a single property Elapsed.
  • The Microsoft ITimer has a single method bool Change (TimeSpan dueTime, TimeSpan period)

@ITaluone
Copy link
Contributor

maybe this mean we should either rename our ITimer to something else

which would be a breaking change?

@dennisdoomen
Copy link
Member

which would be a breaking change?

Correct. Although that interface is only public because of technical constraints. It's not supposed to be used outside the library.

@ITaluone
Copy link
Contributor

It's not supposed to be used outside the library.

What's supposed and how it's used can differ 😉

@dennisdoomen
Copy link
Member

What's supposed and how it's used can differ

That was my point. Can we somehow make it internal? If not, I think we should rename it so that we can help our consumers by avoiding naming conflicts.

@vbreuss
Copy link
Contributor Author

vbreuss commented Nov 19, 2023

I had a look at the usage of the ITimer. The main use cases I found were:

  • NotThrowAfter-Extensions
    It is used to check if the waitTime is already elapsed and the while loop should be terminated
  • ExecutionTime class
    It is used to determine the execution time of the callback

In both cases, it does not use recurring events, so it is not the normal use case for a Timer. Also the default implementation uses a Stopwatch (see StopwatchTimer).

For me the naming is a bit misleading, so if we want to rename the ITimer class, I would rename it to IStopwatch.

@jnyrup
Copy link
Member

jnyrup commented Nov 19, 2023

which would be a breaking change?

Correct. Although that interface is only public because of technical constraints. It's not supposed to be used outside the library.

Isn't our ITimer well enough hidden under the FluentAssertions.Common namespace such that we don't have to change anything? 🤔

@vbreuss
Copy link
Contributor Author

vbreuss commented Nov 19, 2023

Isn't our ITimer well enough hidden under the FluentAssertions.Common namespace such that we don't have to change anything? 🤔

I had to add the following code to the FakeClock class, as it used the ITimer interface:

#if NET8_0_OR_GREATER
using ITimer = FluentAssertions.Common.ITimer;
#endif

So if a consumer does not use the ITimer interface, he is not affected by the ambiguous reference. And if he does use it, a rename would be a breaking change for him.
I think it is not a big issue either way. As it is intended to be used internally, I think the risk of a rename quite small. On the other side, the ambiguous reference is also quite a niche problem.

If you think it a good idea, I am happy to create a pull request to rename it to IStopwatch.

@jnyrup
Copy link
Member

jnyrup commented Nov 19, 2023

My current vote is on doing nothing.
Every new type potentially clashes with other types, that's why we have namespaces.

Renaming to IStopWatch shifts the potential conflict from being with .NET 8 BCL to e.g. System.Reactive.

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Nov 19, 2023

Or rename it to something specific to this lib (which is weird if someone else uses it)?

Like IFluentAssertionsStopwatch

@dennisdoomen
Copy link
Member

Isn't our ITimer well enough hidden under the FluentAssertions.Common namespace such that we don't have to change anything

Yes and no. More types than I expected are under the FluentAssertions.Common namespace. For example, Configuration, CSharpAccessModifier, DateTimeExtensions, IClock, IConfigurationStore, Services and ValueFormatterDetectionMode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation feature
Projects
None yet
Development

No branches or pull requests

5 participants