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

Simpler syntax for fillDescriptions items with limited choices #30932

Closed
kpedro88 opened this issue Jul 27, 2020 · 10 comments
Closed

Simpler syntax for fillDescriptions items with limited choices #30932

kpedro88 opened this issue Jul 27, 2020 · 10 comments

Comments

@kpedro88
Copy link
Contributor

I recently learned (in review of #30850) that fillDescriptions can restrict the allowed choices for a configuration parameter using ifValue(). However, the syntax for this is currently a bit clunky:

  desc.ifValue(edm::ParameterDescription<std::string>("mode", "PseudoAsync", true),
      "Sync" >> edm::EmptyGroupDescription() or
      "Async" >> edm::EmptyGroupDescription() or
      "PseudoAsync" >> edm::EmptyGroupDescription());

This syntax is intended to allow for switching between more complex parameter groups. However, in the case of a simple choice (such as above), it would be nice to have a simpler syntax. For example, the allowed choices could just be provided as a vector.

@kpedro88
Copy link
Contributor Author

assign core

@cmsbuild
Copy link
Contributor

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

A new Issue was created by @kpedro88 Kevin Pedro.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor

Here is an earlier discussion on the topic https://hypernews.cern.ch/HyperNews/CMS/get/progQuestions/331.html

@Dr15Jones
Copy link
Contributor

How about

desc.ifValue(edm::ParameterDescription<std::string>("mode", "PseudoAsync", true),
             edm::allowedValues("Sync", "Async", "PseudoAsync"));

?

In a quick test I was able to make that work. All it needed was the new templated function edm::allowedValues which does exactly what your earlier code did but behind the scenes.

@kpedro88
Copy link
Contributor Author

That looks good to me.

@makortel
Copy link
Contributor

@Dr15Jones How necessary the desc.ifValue() is? (for a quick solution it probably is)

I'm just wondering if we should think about a more general mechanism to specify constraints on allowed values.

@Dr15Jones
Copy link
Contributor

See #30950

@makortel
Copy link
Contributor

+1

Done in #30950

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

@qliphy qliphy closed this as completed Feb 11, 2021
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

5 participants