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 fmt::formatter for Options #2341

Merged
merged 11 commits into from
Jun 8, 2021
Merged

Add fmt::formatter for Options #2341

merged 11 commits into from
Jun 8, 2021

Conversation

ZedThree
Copy link
Member

@ZedThree ZedThree commented Jun 3, 2021

Options can now be converted to a string with fmt::format with the following format options:

  • 'd': include 'doc' attribute if present
  • 'i': inline section names
  • 'k': only print the key, not the value
  • 's': include 'source' attribute if present

(happy to bikeshed these, but this vague set is useful)

This allows us to replace both toString(Options) and OptionINI::write with calls to fmt::format instead.

The unused inputs error can now print out the type and docstring, e.g.:

Unused option 'nput', did you mean:
        nout            # type: int, doc: Number of output steps
        solver:nout             # type: int, doc: Number of output steps. Overrides global setting.
        nz              # type: int
        MXG             # type: int, doc: Number of guard cells on each side in X
        MYG             # type: int, doc: Number of guard cells on each side in Y
        MZ              # type: int
        ZMAX            # type: BoutReal
        ZMIN            # type: BoutReal
        grid            # type: string
        phisolver:dst           # type: bool

which uses fmt::format("\t{:idk}\n", suggestion) for each suggested alternative.

This will also be helpful for #2336, being able to print the docstring

This means that

   bool = options["foo"];

will still work, while

   dcomplex = options["foo"];

will need to be changed to:

   dcomplex = options["foo"].as<dcomplex>();

This is needed so that we can pass `Options` to templated functions
and avoid ambiguous conversions, etc.
Previously `fmt::format("{}", options["value"])` was not handled
correctly, as the formatting implicitly assumed the `Options` was a
section
@ZedThree ZedThree added this to the BOUT-5.0 milestone Jun 3, 2021
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

include/options.hxx Show resolved Hide resolved
include/options.hxx Show resolved Hide resolved
include/options.hxx Show resolved Hide resolved
include/options.hxx Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/sys/options.cxx Outdated Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
src/sys/options.cxx Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2021

clang-tidy review says "All clean, LGTM! 👍"

bendudson
bendudson previously approved these changes Jun 8, 2021
Copy link
Contributor

@bendudson bendudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ZedThree It would be nice to have something in the manual on this, if you have time

Copy link
Contributor

@bendudson bendudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ZedThree !

@ZedThree ZedThree merged commit e4de8bd into next Jun 8, 2021
@ZedThree ZedThree deleted the fmt-format-options branch June 8, 2021 16:25
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.

None yet

2 participants