Skip to content

Fluent APIs are not allowed in non-builder types #1994

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 15, 2022
Merged

Fluent APIs are not allowed in non-builder types #1994

merged 8 commits into from
Dec 15, 2022

Conversation

adamsitnik
Copy link
Member

Based on the feedback from @bartonjs

@adamsitnik adamsitnik requested a review from jozkee December 15, 2022 15:51
@KalleOlaviNiemitalo
Copy link

Should the ArgumentValidation.AcceptExistingOnly and OptionValidation.AcceptExistingOnly methods likewise return void?

jozkee
jozkee reviewed Dec 15, 2022
View reviewed changes
@adamsitnik @jozkee
Apply suggestions from code review
f2d5d2f 
Co-authored-by: David Cantú <dacantu@microsoft.com>
@adamsitnik adamsitnik requested a review from jozkee December 15, 2022 16:31
jozkee
jozkee reviewed Dec 15, 2022
View reviewed changes
src/System.CommandLine.Tests/OptionTests.cs Outdated
@@ -372,10 +372,8 @@ public void Option_of_enum_can_limit_enum_members_as_valid_values()
[Fact]
public void Option_of_T_fluent_APIs_return_Option_of_T()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might as well remove the test

jozkee
jozkee reviewed Dec 15, 2022
View reviewed changes
src/System.CommandLine.Tests/ParsingValidationTests.cs
@@ -672,9 +690,12 @@ public void LegalFileNamesOnly_rejects_option_arguments_containing_invalid_file_
[Fact]
public void LegalFileNamesOnly_accepts_command_arguments_containing_valid_file_name_characters()
{
Argument<string[]> argument = new ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
Argument<string[]> argument = new ();
Argument<string[]> argument = new();

jozkee
jozkee approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the ArgumentValidation.AcceptExistingOnly and OptionValidation.AcceptExistingOnly methods likewise return void?

Up to Adam's choice, but that could be addressed in a follow-up PR if required.

@adamsitnik
Copy link
Member Author

Should the ArgumentValidation.AcceptExistingOnly and OptionValidation.AcceptExistingOnly methods likewise return void?

That is a very good question. To be honest I don't like AcceptLegalFilePathsOnly and AcceptLegalFileNamesOnly as they don't restrict the argument/option type in any way. Example:

Option<bool> flag = new ("--flag");
flag.AcceptLegalFilePathsOnly(); // it should work only with files, not things like boolean

ArgumentValidation.AcceptExistingOnly and OptionValidation.AcceptExistingOnly restrict the types, but our BCL guidelines are against introducing extension methods for types that we own (they should be just instance methods). When @bartonjs and @jonsequitur are back from holidays I am going to discuss it with them.

@adamsitnik adamsitnik merged commit c33ead5 into dotnet:main Dec 15, 2022
@adamsitnik adamsitnik deleted the returnVoid branch December 15, 2022 18:29
@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this pull request Jan 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jozkee jozkee jozkee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamsitnik @KalleOlaviNiemitalo @jozkee