How about a HasFlag() extension method just like a regular enum?

[syedsuhaib]

I know that @AFM-Horizon is already working (#64) on a SmartFlagsEnum concept for this but I couldn't add an issue on his fork.

HasFlag() is one of the most useful extensions on good old enums. I suppose it wouldn't take much to add that in, given the well-laid out structure of SmartFlagsEnum by @AFM-Horizon.

I would go so far as to have AddFlag() and RemoveFlag() extension methods too, which I have so desperately wished regular enums had. I end-up writing my own AddFlag() and RemoveFlag() methods for most projects where I use enums.

Any thoughts?

[AFM-Horizon]

Hi @syedsuhaib! How would a HasFlag() method usually operate?

[syedsuhaib]

You can have a look at the Official Documentation of HasFlag. In a nutshell, it returns a simple boolean value indicating whether an instance has a flag (or set of flags) set or not. We simply have to return (this & flag) == flag unless you want to treat the case where current instance is zero, differently. Because that will return true for any flag values passed.

[AFM-Horizon]

@syedsuhaib Do the TryFromValue() and the TryFromValueToString() methods provide similar functionality as HasFlag()? Although they return values as well as a bool they may provide a good point to modify and create two HasFlags methods (one for strings and one for value). (Let me know if I'm on the right track here!) I like the AddFlag() and RemoveFlag() methods to, however, Am I right in understanding that this would add values to the SmartFlagEnum class itself? If so this may require some sort of runtime code generation to change the class and add in more values.

[syedsuhaib]

No they're not the same. TryFromValue() and TryFromValueToString() are both static methods. In HasFlag(), we're looking at instance methods which will do checks on the value stored in that instance _value. No need to play around with the string representing it.

[AFM-Horizon]

Ohhh sorry I think I understand! It would operate the value itself to see if it represents within it the flag bit value you are looking for. (Possibly still a little confused, But i catch your drift!) Sounds great. I'm sure it will make more sense when I see it working! Cheers!

[syedsuhaib]

I was trying to see how I'd go about this HasFlag() and then I noticed that your SmartFlagEnum.FromValue() is returning an IEnumerable<TEnum>. I don't think that's how Flags should work. You're still supposed to return a simple TEnum, just like SmartEnum.FromValue() does. The difference between SmartEnum and SmartFlagEnum would only be visible when doing FromName() or ToString(). If you need IEnumerable<TEnum> for some internal manipulations, you need to have a separate private member for that purpose. But the public member should always return just the obvious TEnum parsed from the supplied values.

[syedsuhaib]

This, by the way, makes me question the whole point of having a separate SmartFlagEnum class for #64. If the differences are so few, we could just introduce some conditional logic in the original SmatEnum class itself based on a custom flag. Using Flags would introduce unnecessary dependency on System.Runtime (or are we already dependent on that?!). Besides, FlagsAttribute seems to be restricted by AttributeUsage(System.AttributeTargets.Enum) which would disallow us from using it for SmatEnum. I'm sorry if I'm not comprehending something else that we're achieving by having a separate class. But I say get rid of all that checking for sequential powers of two, or even powers of two to begin with. I have sometimes preferred to have a higher power of two defined while leaving lower powers undefined. Leave that much discretion to the user, if they know what they're doing. Albeit, having powers of two will remain the recommended way of defining members, just like regular enums. In fact enforcing powers of two is wrong because that way we're preventing the users from defining enumerated constants for commonly used flag combinations.

If it's a Flags SmartEnum, we just don't throw the ValueNotFoundException() blindly. We throw only if the value can't be cast to any combination of the defined flags. Maybe you're already doing this, I didn't check. But just saying in case we decide to port back to SmartEnum class.

[AFM-Horizon]

@syedsuhaib @ardalis, @sdepouw I see what you are saying. I was the only one working on this (I also did run it past a flag enum user and she agreed that it should return an Ienumerable!) So It's entirely possible I got confused as to what the FromValue() method should do. I was under the impression that it should work the way I defined it. If it's only meant to return a single value then likely I have wasted my time hahaha! I would have thought that was the entire purpose of a SmartFlagEnum! so that you could get back actual instances of the class for a value which may have other functionality defined on their class instance. I thought this to be doubly true due to the fact that this enum has actual classes as the value for each enum instance, if that makes sense at all. (see my version of the README.md ) that explains how this would work.

If anyone would like to weigh in this is probably important to clear up at this point before sinking in anymore work!

Thanks!

[AFM-Horizon]

@syedsuhaib @ardalis @sdepouw In addressing your second point. Yes If we don't want to return IEnumerable we could probably just remove the SmartFlagEnum class and create some helper methods to provide the "Flag" functionality. Although this (again in my opinion would negate the purpose of a "SmartEnum") turning into a simple old enum (DumbEnum?) lol.
Another reason I created a second class is that many of the methods are static and provided a real pain point in overriding to create the IEnumerable functionality. again this decision was contingent on the point above.

In response to your point about the sequential powers of two. I thought that people may feel that way about guide rails both in terms of Performance and Opinion. So they are optional through the [AllowUnsafeEnumAttribute] perhaps this should be off by default and the logic should be switched so it is a [ProtectAgainstUnsafeEnumValues]. which would be easy enough to fix!

In terms of the power of two checking I should point out that I also attempted to write the algorithm to allow for enumerated constants (that what they are called! I was calling them explicit flag enum values) firstly so that an exception is not thrown if they are provided and secondly so that they will be returned rather than the multiple enum values that would be returned if they are not explicitly provided. (Further testing and work may be required here to ensure all of the specific use cases are working as required however! for instance I believe the specific use case of providing a constant enumerated value without it's constituent power of two values will cause an exception to be thrown see AllowUnsafeFlagEnumAttributeTests.cs and feel free to add the test for the specific scenario you are wanting to check). Remember this is all optional anyway, and is simply provided as a guide rail for beginners (like myself :))

Finally I believe we're only throwing the ValueNotFound exception if a explicit constant value OR combination value is not found for the value.
Hopefully this along with the README.md (there is a dedicated section for SmartFlagEnums) explains some of the decisions I made and what some of the issues were surrounding those decisions. Now is the perfect time to figure all this stuff out before it gets released! Cheers Alex

[AFM-Horizon]

@syedsuhaib @arootbeer @sdepouw @ardalis @sgoodgrove (not sure who here is still active so I just @tted some names!) Hi guys just checking in does anyone else want to weigh in on this issue as I'm now unsure of the design and whether it is implemented correctly in terms of functionality. Should the SmartFlagEnum return only a single value? or an IEnumerable containing the flag values that make up the combination? I want to push on, but first It would be good to get some more opinions on the correct functionality! Thanks everyone!

[syedsuhaib]

I was also waiting and hoping others would take a fresh look at this, perhaps we're both biased because of our direct involvement. I reckon it may not be as straightforward as I am seeing it currently. In my opinion, we just save the value whatever passed to an instance in the _value without much validation beyond basic type compatibility. As for the name, that's where the binary flag checking logic should kick in to work out the comma separated list of flag names.

This can of course be achieved by concatenating the names from an IEnumerable as well, but it's just that I don't see the point of doing that, if all we want is a string at end. Beyond that, we'll have the proposed HasFlag(), AddFlag(), and RemoveFlag() methods for logical manipulations with the set flag enum values.

That's what my take is right now. Perhaps @ardalis can look at both views and help us choose a direction to take this forward. This is of course going to be one of the most significant enhancements of SmartEnum.

[ardalis]

I've been remiss in providing much leadership on this because I don't use this feature (flags enums) very often. When I do, with the standard enum feature, I've been fine to just OR together a set of flags and do a boolean test to see if it matches a given flag. Listing all of the "ON" flags isn't something I've generally needed to do that I can recall.

My hope has been that "the community" would arrive at a good solution that I could then pull in, but it seems we may be at a bit of an impasse on that, with a couple of competing good solutions. I've also been really busy with other things so this hasn't been top of mind. Let me try and make it a priority to look at this and get some other eyes on it and one way or the other implement the functionality in a new release of the package, if it makes sense to do so.

[AFM-Horizon]

Thanks Steve!
I have never used flag enums at all! This being the case I am not overly confident that the solution I coded is the correct one. (Although it's pretty cool :) ) I am happy to go with whatever option you guys decide on. Much of what I have coded is based around the multiple value return concept, The code can be much more streamlined if this is not a requirement and indeed may simply be an extension method of the main SmartEnum rather than a separate library worth of work as @syedsuhaib has said above!!
Cheers everyone.