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

Add helper for argument validation #1561

Open
pascalberger opened this issue Apr 3, 2017 · 55 comments
Open

Add helper for argument validation #1561

pascalberger opened this issue Apr 3, 2017 · 55 comments

Comments

@pascalberger
Copy link
Member

Each Cake addin requires some kind of argument validation (or at least should :). Instead of having to implement this in every addin it would be nice if this could be provided for common cases (null check, empty string check, ...) by Cake.Core.

@pascalberger
Copy link
Member Author

This could also be helpful Cake internally to simplify argument checking, like its eg done here.

If this is of interest I can provide a PR for a class similar to the one which we use in the Cake.Prca Addin.

@gep13
Copy link
Member

gep13 commented Apr 24, 2017

@pascalberger I would have no objection to this.

@cake-build/cake-team thoughts?

@phillipsj
Copy link
Contributor

phillipsj commented Apr 25, 2017

This sounds pretty awesome if it makes it easier. It is a common pattern in every addin.

@daveaglick
Copy link
Member

I agree, this would be great. There's already several argument validation frameworks - I wonder if any of them have source-only NuGet packages...

@agc93
Copy link
Member

agc93 commented Apr 25, 2017

Agreed that it would be good to centralise this (few other candidates for that) 👍

@daveaglick I've used Ensure.That from @danielwertheim before, but looks like the source-only version hasn't gotten the latest updates yet..

@bjorkstromm
Copy link
Member

Agreed!

But instead of signature void Check<T>(T...) I would rather see T Check<T>(T...). Then you could write checks like

_foo = Check.NotNull(foo);

I personally think ASP.NET team have a quite decent boilerplate here which I have used from time to time.

@agc93
Copy link
Member

agc93 commented Apr 25, 2017

Additional reason to centralise this I just noticed: the Cake.Prca validation examples @pascalberger linked to currently show up on just about every type in the documentation, which could be a bit misleading, even with the From PrcaArgumentChecks)

@patriksvensson
Copy link
Member

I like the link @mholo65 posted.

@pascalberger
Copy link
Member Author

OK, I'll create a PR containing a class based on the ASP.NET boilerplate.

@patriksvensson
Copy link
Member

@pascalberger Just make sure to attribute ASP.NET project in your code.

@pascalberger
Copy link
Member Author

Since there's currently only interest for aliases providing argument checking but not also for a public class which can be consumed in addins, I'll leave this to someone else for implementing (or there is also already an implementation for such aliases in Cake.Incubator).

@bjorkstromm
Copy link
Member

Adding Up-for-grabs on this one.

This is what we thought of:

  • Add internal static class for argument checking. You could use this and attribute ASP.NET team in the comments. This file should be sourced in every project (<Compile Include="..\Shared\Check.cs" />)
  • Change argument checking to use new approach. E.g. change this:
if (foo == null)
{
    throw new ArgumentNullException(nameof(foo));
}
_foo = foo;

to:

_foo = Check.NotNull(nameof(foo));

@patriksvensson
Copy link
Member

@mholo65 Perhaps we should reserve this one for https://yourfirstpr.github.io/? I could do the mentoring.

@bjorkstromm
Copy link
Member

@patriksvensson sounds good to me!

@bookman220
Copy link

@patriksvensson I'd be interested in trying this out

@patriksvensson
Copy link
Member

@bookman220 Great! Do you want any help to get started or do you want to give it a shot yourself?

@bookman220
Copy link

@patriksvensson I'll probably need some help but let me figure out how to build this thing first.

@sarithsutha
Copy link

@patriksvensson @bookman220 I don't want to step on anyone's toes, I'm interested in trying this out if it is still up for grabs. This would be my first time contributing to open source.

@bookman220
Copy link

@sarithsutha @patriksvensson I do apologize for taking so long. I do think I may need some guidance however. Do I just open up the solution file in visual studio and compile it from there? If I haven't fixed the big within the next week you may take it sarithsutha

@sarithsutha
Copy link

@bookman220 thanks for your response. No rush, take your time, I don't mind waiting. I use Visual Studio 2017, I was able to just open the solution and compile it from there. And with VS code I was able to build using the Cake addin.

@bookman220
Copy link

@sarithsutha It appears I am having some trouble building the project. Apologies for needing such basic advice but I am new to Visual Studio and C#. It appears a fair amount of the solution failed to load and when I go to Build it just has the option to run a code analysis on the solution, which I did and you can see the output. Can you give me some pointers as to where to go from here?
image

@patriksvensson
Copy link
Member

@bookman220 There seems to be problems loading the different projects according to the screenshot.

The following prerequisites need to be installed:

The build.cake file is only used to build the project from command line. When the project is loading correctly in Visual Studio, you should be able to build everything from the Build menu.

@micheleissa
Copy link

@patriksvensson I'd like to take a try on this one.

@micheleissa
Copy link

What I'm planning to do :

  • Add folder called 'Shared' in the solution directory.
  • Add 'Check.cs' inside that folder.
  • Modify each csproj file to include
<ItemGroup>
    <Compile Include="..\Shared\Check.cs" />
</ItemGroup>
  • The namespace for it Cake.Shared
  • I saw the link from ASP.NET team but I feel it is more complicated than it should be and I was thinking of something along the following
public static T NotNull<T>(T o, string message = null)
            {
            if (o == null)
                {
                throw new ArgumentNullException(message ?? $"{nameof(o)} cannot be null");
                }
            return o;
            }

Thoughts?

@kcamp
Copy link
Contributor

kcamp commented Oct 27, 2017

As a developer, it would be preferable to more correctly support parameter names; nameof(o) is not going to be semantically useful in the exception that's thrown.

Reference:
https://msdn.microsoft.com/en-us/library/k8a0dfcy(v=vs.110).aspx

Consider something like this, or overloads that would support the same..

T NotNull<T>(T item, string parameterName, string message = null)  { .... }

So that the calling convention would be more akin to

public SomeConstructor(object arg) 
{
    _fieldName = ValidationHelperClassName.NotNull(arg, nameof(arg));
    ....

@patriksvensson
Copy link
Member

@micheleissa This is still assigned to @bookman220.
@bookman220 Are you still working on this?

@micheleissa
Copy link

@kcamp
What would you use parameterName for ?
in the case someone passes 'bogus' as the parameterName and it is used in the exception message the result will not be accurate in my opinion.

@patriksvensson
Copy link
Member

@micheleissa What @kcamp means is that nameof(o) will always print o. It will never used the original parameter name.

@micheleissa
Copy link

@patriksvensson gotcha. Silly me!

@micheleissa
Copy link

So are you supposed to wait for a response from 'bookman220'

@patriksvensson
Copy link
Member

@micheleissa let’s give him/her a couple of days to respond before reassigning it.

@micheleissa
Copy link

sounds good.

@bookman220
Copy link

@patriksvensson @micheleissa Again I apologize for being so obtuse, but to be honest I am a little disheartened by how flummoxed I am by the environment, which is why it has taken me so long. I work with Javascript scripts in my day job and have debugged compilation errors in C# to get a game running using the Unity game engine, but for some reason I'm finding myself staring slack jawed at this even though it should be simple. At anry rate when I try to build it just says run code analysis on solution, and then in the output below it's all zeros:
image

@bookman220
Copy link

By the way I downloaded the .NET 2.0 SDK and I already had Visual Studio 2017, though it updated me to the latest version when I ran the installer.

@bookman220
Copy link

As you can see from my other solution for the games, I'm having no trouble there:
image

@bookman220
Copy link

Apologies for forgetting to crop out the second screen by the way.

@kcamp
Copy link
Contributor

kcamp commented Oct 30, 2017

@bookman220 What happens when you run the build [via Cake] from Powershell?

c:\git\cake > .\build.ps1

@bookman220
Copy link

Here's the error I get:
image

@bookman220
Copy link

@kcamp @patriksvensson Do you guys have any suggestions?

@patriksvensson
Copy link
Member

@bookman220 You seem to have a custom nuget source pointing at myget-2e16-kxcdn.com. Have you removed the default nuget source?

@bookman220
Copy link

@patriksvensson I would have done no such thing intentionally. Any tips on how I might restore the default one?

@jnm2
Copy link
Contributor

jnm2 commented Nov 18, 2017

Maybe find the config file that is overriding the defaults? https://docs.microsoft.com/en-us/nuget/consume-packages/configuring-nuget-behavior#config-file-locations-and-uses

@qpooqp
Copy link

qpooqp commented Jun 2, 2018

Hello.
I can see that there was no activity here for a long time so can i take care of this?

Can you just point out some checks which would be usefull to implement?
Here are some which could be there:

  • (for generic) NotNull
  • (for strings) NotEmpty, Empty, ShorterThan, LongerThan, LengthBetween, MatchRegularExpression, StartsWith, EndsWith, Contains, EqualsTo
  • (for collections) NotEmpty, Contains, Any, All
  • (for numbers) LessThan, MoreThan, Positive, Negative
  • (for booleans) IsTrue, IsFalse

@bookman220
Copy link

@qpooqp Did you tackle this one?

@qpooqp
Copy link

qpooqp commented Nov 16, 2018

To be honest i forgot about this one.
But i will try to do it ASAP.

@bookman220
Copy link

@patriksvensson I have installed the NuGet Package Manager and am trying to see if "Sync-Package" would fix the issue. Do you know what value I should supply for the "Id" parameter here?
image

@patriksvensson
Copy link
Member

@bookman220 I’m afraod that I have no clue what sync-packages does.

If you’re trying to restore packages for the solution it should be enough to do a rebuild.

@bookman220
Copy link

@patriksvensson When I go to "Build" I only see "Run Code Analysis on Solution" and when I select that I get "========== Rebuild All: 0 succeeded, 0 failed, 0 skipped =========="
image
image

@patriksvensson
Copy link
Member

@bookman220 I have no idea why that occurs to you.

Just to double check, do you open ./src/Cake.sln or did you open something else?
What version of Visual Studio are you using? The latest VS2017 version is recommended.

@bookman220
Copy link

@patriksvensson

  1. Yes, I'm opening ./src/Cake.sln:
    image
  2. I had been running a slightly older version (14 something) but now I'm all caught up and have the same issue:
    image

@bookman220
Copy link

Here are the .NET components I have installed:
image

@devlead
Copy link
Member

devlead commented Dec 2, 2018

You'll need the .NET Core sdk bits too.

@bookman220
Copy link

@devlead Ok now I have the 4.7.2 .NET SDK (added the targeting pack for good measure):
image
One time when I opened it an alert box popped up saying "One or more projects in this solution did not load correctly. See Output for more information", however I cannot reproduce this behavior. Here's what I'm seeing when I open the solution:
image

@bookman220
Copy link

@patriksvensson I happened to be reading an article on Docker containers and I wondered if that could help people with creating an environment in which myself and others could build the app: https://www.sumologic.com/blog/devops/kubernetes-vs-docker/

@OhYash
Copy link

OhYash commented Oct 21, 2019

Hey is this one done?
@pascalberger posted AssertExtensions.cs link, which seems to contain the said assertion functions. Still asking cause the issue is open at the moment.

To everyone greeted with 404 error on hitting up the ASP.NET example, the updated location is here.

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

No branches or pull requests