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 multi-instance option support globally and per option #625

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

geckosoft
Copy link

This PR continues on the work done in PR #594 and makes the following changes:

  • Renames the parser setting to AllowMultiInstanceByDefault
  • Add a per-option AllowMultiInstance option that can be set in the constructor (by default, each option looks at the AllowMultiInstanceByDefault parser setting)
  • Added 2 additional tests (1 for the AllowMultiInstance option)
  • Previously enabling AllowMultiInstance(ByDefault) would disable any 'EnforceSingle' check, now it will perform all checks again but at the final step we look at the AllowMultiInstanceByDefault parser setting and the AllowMultiInstance option

Any feedback is welcome.

tydunkel and others added 2 commits March 13, 2020 16:20
Renamed the global AllowMultiInstance setting to AllowMultiInstanceByDefault as this value is used to define the default action to take when the per-option AllowMultiInstance isn't specified
Added extra test to test the AllowMultiInstance option on options
Added extra test to make sure parsing fails on normal non(null)-AllowMultiInstance options with the global setting set to false as expected
@rmunn
Copy link
Contributor

rmunn commented May 12, 2020

While I'm not the maintainer, nullable boolean arguments don't fit with the code style. If you really have to have a three-valued bool (true, false, or not specified) for allowMultiInstance in OptionSpecification, I'd suggest using a Maybe<bool> instead and the appropriate Maybe-handling functions from Csharpx, as found in many other parts of the code. But my personal preference would be to just say "not specified = false" and use a regular boolean.

@mylemans
Copy link

mylemans commented May 12, 2020

Doing the not specified = false would make it impossible to enable multi instance by default and opt-out per option though.

Also, if I read Maybe correctly, if I'd use Maybe.Return(false) it would actually return an instance of Nothing

@rmunn
Copy link
Contributor

rmunn commented May 12, 2020

Ah, right. Then a Maybe<bool> is the right approach here. And no, Maybe.Return(false) should not return a Nothing instance, at least if it's implemented properly. I'll check.

@rmunn
Copy link
Contributor

rmunn commented May 12, 2020

Okay, looks like there's a bug in Maybe. The Return function is supposed to always return a Just instance instead of Nothing, but the way it was implemented, you're right that it would turn a false into a Nothing, incorrectly. To work around that, use Maybe.Just instead of Maybe.Return when you know the value should be Just(false).

@mylemans
Copy link

I'll see if I can make the requested changes. I'm not the biggest fan of using Maybe for this, but I'll see if I can comply.

@mylemans
Copy link

mylemans commented May 12, 2020

@rmunn Hmm, Maybe isn't public so I cannot use this type for the public AllowMultiInstance read-only option on OptionAttribute, should I make that property internal?

edit: Maybe I should retain the nullable bool there and change everywhere else to use this 'Maybe'? Like in FromAttribute and OptionSpecification

@rmunn
Copy link
Contributor

rmunn commented May 12, 2020

If it's a property on OptionAttribute, then it has to be public, since that's what library users will be accessing when they write Option("foo", AllowMultiInstance=false). BTW, I believe it should be a read-write property so that library users can set it on the options, like with the Min and Max properties in BaseAttribute. And yes, perhaps a nullable bool on the allowMultiInstance member of OptionAttribute would fly. For the property, though, I suggest that it should be a simple bool, whose getter should calculate the effective value (based on whether it was set or not, and based on the default setting if it wasn't set) and whose setter should set the member.

@mylemans
Copy link

AllowMultiInstance cannot be a nullable bool then though (not allowed in attributes).

Making it only set the nullable bool internally on set could work (be it false or true) but only for writing.

For calculating the effective value, you will need the current parser' context to get that setting, not sure how you would feed that info to the option?

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

Successfully merging this pull request may close these issues.

4 participants