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

Treat .NET 7 required properties as actually required by default #862

Open
DoctorKrolic opened this issue Dec 8, 2022 · 4 comments
Open

Comments

@DoctorKrolic
Copy link

Code snippet:

public class Options
{
    [Option]
    public required int Port { get; init; }

    [Option]
    public required string Server { get; init; }
}

Expected behaviour: Both Port and Server are required and the parsing fails if at least on of them is not present.

Actual behaviour: They are not required, I still have to specify [Option(Required = true)] in order for it to work

@Orace
Copy link

Orace commented Dec 12, 2022

Using sharplab.io, it appears that a RequiredMemberAttribute is added to the required properties.

Search for this attribute is a trivia.

@Orace
Copy link

Orace commented Dec 13, 2022

Has mentioned by DoctorKrolic here

What if you have a required modifier and [Option(Required = false)]? I would say, it should throw because of conflicting settings. Please add test for such case

Required = false is the default case, hence, if we implement this, it will throw on default case.

  1. We can make Required = true mandatory on required properties, which is redundant and against the purpose of this issue (Treat .NET 7 required properties as actually required by default).

  2. We can change the type of the Required attribute property to a try-state (unset, true, false) with an unset default value, which will be treated:

    • as true in the case of a required property
    • as false in other cases
  3. We can stay silent and ignore Required = false on required properties.

Option 2 looks pretty, but we must validate that it is not a breaking change (see this Jon Skeet post where he talk about the optional parameters). Does a bool? do the trick?

@DoctorKrolic
Copy link
Author

Required = false is the default case, hence, if we implement this, it will throw on default case.

Uh, I see...

Does a bool? do the trick?

Having Required as a bool? can really be a win, so we infer null state to true if the property is required and false otherwise. I am afraid, this is a binary breaking change though...

@Orace
Copy link

Orace commented Dec 13, 2022

A quick look to the IL shows it unfortunately is 😒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants