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

ElapsedEventArgs doesn't have a public constructor #31204

Closed
ashelleyPurdue opened this issue Oct 15, 2019 · 8 comments · Fixed by #94489
Closed

ElapsedEventArgs doesn't have a public constructor #31204

ashelleyPurdue opened this issue Oct 15, 2019 · 8 comments · Fixed by #94489
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.ComponentModel
Milestone

Comments

@ashelleyPurdue
Copy link

System.Timers.ElapsedEventArgs doesn't have a public constructor, despite just being a data class containing one property. This makes it unnecessarily awkward to write a mock for something that uses an ElapsedEventHandler, because you need to use a workaround just to create this one object.

It's impossible to solve this by subclassing ElapsedEventArgs, because the SignalTime property is read-only and therefore cannot be set by your constructor.

@scalablecory scalablecory transferred this issue from dotnet/core Oct 17, 2019
@vcsjones
Copy link
Member

vcsjones commented Oct 17, 2019

To put this in terms of a concrete API proposal:

namespace System.Timers
{
    public class ElapsedEventArgs : EventArgs
    {
+       public ElapsedEventArgs(DateTime signalTime);
    }
}

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@ericstj ericstj modified the milestones: 5.0.0, 6.0.0 Jul 9, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jul 9, 2020
@ericstj
Copy link
Member

ericstj commented Jul 9, 2020

This seems like a reasonable request.

@ericstj
Copy link
Member

ericstj commented Jul 12, 2021

At this point it's too late for this to go into 6.0.0.

@ghost ghost moved this from 6.0.0 to Future in ML, Extensions, Globalization, etc, POD. Jul 12, 2021
@safern safern removed their assignment Feb 22, 2022
@JasonDWilson
Copy link

Did this make it into 7? It really is a pain

@vcsjones
Copy link
Member

Did this make it into 7?

It has not. Since this issue was originally filed, #44853 changed the constructor to take a DateTime instead of a long. So now it really is just a matter of making the constructor public.

@dotnet/area-system-componentmodel this seems like an easy enough one I can take care of if we want to put this through API review. Proposal at #31204 (comment)

@steveharter steveharter modified the milestones: Future, 9.0.0 Oct 27, 2023
@steveharter steveharter added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 27, 2023
@terrajobst
Copy link
Member

terrajobst commented Nov 7, 2023

Video

  • Currently people can't derive from this type (because there are no public constructors), we should seal it
  • Do we need to add any argument validation?
namespace System.Timers;

public sealed class ElapsedEventArgs : EventArgs
{
    public ElapsedEventArgs(DateTime signalTime);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 7, 2023
@TheMaximum
Copy link
Contributor

I'd be interested to make this change. A quick look at the existing class shows there's already an internal constructor taking a DateTime argument (see below), wouldn't it be sufficient to change this to public (and seal the class)?

public class ElapsedEventArgs : EventArgs
{
internal ElapsedEventArgs(DateTime localTime)
{
SignalTime = localTime;
}
public DateTime SignalTime { get; }
}

@vcsjones
Copy link
Member

vcsjones commented Nov 7, 2023

wouldn't it be sufficient to change this to public (and seal the class)?

Yep.

Do we need to add any argument validation?

We discussed this further in the API review and we didn't think of any reasonable validations that should be applied as the input.

TheMaximum added a commit to TheMaximum/dotnet-runtime that referenced this issue Nov 7, 2023
Changed the internal ElapsedEventArgs constructor to be public, sealed
the class, and renamed constructor argument to match property name.

Fix dotnet#31204
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 7, 2023
buyaa-n pushed a commit that referenced this issue Nov 9, 2023
* ElapsedEventArgs: public constructor + sealed

Changed the internal ElapsedEventArgs constructor to be public, sealed
the class, and renamed constructor argument to match property name.

Fix #31204

* ElapsedEventArgs: added missing XML comment

---------

Co-authored-by: Max Klaversma <max.klaversma@vandoren.nl>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.ComponentModel
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants