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

[Params] for enums should include all values by default #658

Closed
svick opened this issue Feb 22, 2018 · 5 comments
Closed

[Params] for enums should include all values by default #658

svick opened this issue Feb 22, 2018 · 5 comments
Milestone

Comments

@svick
Copy link
Contributor

svick commented Feb 22, 2018

I think that with [Params] properties whose type is some enum, a fairly common situation is that I want all values of that enum to be included. It would be nice if doing this didn't require listing all those values in the attribute. I.e., instead of having to write:

[Params(CurrentCulture, CurrentCultureIgnoreCase, InvariantCulture, InvariantCultureIgnoreCase, Ordinal, OrdinalIgnoreCase)]
public StringComparison StringComparison { get; set; }

I could write just:

[Params]
public StringComparison StringComparison { get; set; }

Do you think this would be a useful feature? Would this be considered a breaking change? (If yes, something like [EnumParams] could be added instead.)

@adamsitnik
Copy link
Member

I like this idea, but I am not sure how we make it more intuitive. Our users are used to providing all values.

Maybe something like:

[AllEnumValues]
public StringComparison StringComparison { get; set; }

@svick what do you think?

@svick
Copy link
Contributor Author

svick commented Feb 23, 2018

@adamsitnik Yeah, I think that's reasonable (and it's what I meant by [EnumParams]).

Regarding naming, I think that since it's a variation on Params (just like ParamsSource), I think the name should include "Params". But I'm not sure what exactly would be the right name: EnumParams is too cryptic and something like ParamsAllEnumValues is too long.

@adamsitnik
Copy link
Member

@svick so maybe EnumAllParam ?

Naming is hard ;p

@gsomix
Copy link
Contributor

gsomix commented Oct 9, 2018

Hey, @svick, @adamsitnik, @AndreyAkinshin, do you mind if I reopen #664 with additional changes (tests, validator)?

@svick
Copy link
Contributor Author

svick commented Oct 9, 2018

@gsomix Feel free to go ahead. I don't know when I would have the time to do it myself.

gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 13, 2018
gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 13, 2018
gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 13, 2018
@gsomix gsomix mentioned this issue Oct 13, 2018
4 tasks
gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 13, 2018
gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 13, 2018
gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 13, 2018
gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 14, 2018
gsomix added a commit to gsomix/BenchmarkDotNet that referenced this issue Oct 14, 2018
adamsitnik pushed a commit that referenced this issue Oct 14, 2018
* Added [ParamsAllValues]

* #658: restore lost ParamsAllValues attribute

* #658: fix sample

* #658: remove attribute's duplicate

* #658: add tests and validator for ParamsAllValues

* #658: update docs

* #658: remove integration tests

* #658: refactor ParamsAllValues validator

* #658: update docs
@AndreyAkinshin AndreyAkinshin added this to the v0.11.2 milestone Oct 14, 2018
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

4 participants