Request: Custom argument formatting for PrintfFormatter

[spacemoose]

As discussed in issue 331

I would find it very helpful to be able to specify custom argument formatting for the printf interface.

Please let me know if I can help in any way.

[vitaut]

Thanks for the suggestion. Adding custom argument formatter support to PrintfFormatter should be very similar to BasicFormatter (#235). Basically the argument formatter class will have to be passed as an additional template argument. If you could contribute this feature using BasicFormatter as an example that would be great. Otherwise I can look into it myself but this might take a while.

[spacemoose]

Okay, so I hacked out a proof of concept extension that seems to have the desired behavior. I was a little out of my comfort zone here, which means it was rewarding work, but I'd appreciate a review and
suggestions for improvement. I didn't have any great ideas regarding naming so please let me know your preferences there. You can see the work at my fork of the project here: https://github.com/spacemoose/fmt

I'd like some feedback from you regarding what you'd like to see before pulling. Here are some of my thoughts:

1. Am I on the right track? Obviously I should test more.
2. Are you using a published formatting style? Otherwise I'll do my best.
3. I extracted a file printf_formatter.h for the modified code. I put the stuff which was previously in the anonymous namespace into the internal namespace. Reasonable?
4. I'd be inclined to try to split the two interfaces into two header files if you're amenable to that. In my opinion format.h could be split up a little.
5. Like with 235, I guess I should pull the ArgFormatterBase out internal
6. If I do 4, maybe we could make the naming of ArgFormatterBase more consistent with the naming of BasicFormatter?

[vitaut]

[1] Am I on the right track? Obviously I should test more.

Yes, although I haven't tested your changes they do look right.

[2] Are you using a published formatting style? Otherwise I'll do my best.

I'm following Google C++ Style Guide with two exceptions: (1) exceptions are allowed, (2) function names are lowercase.

[3] I extracted a file printf_formatter.h for the modified code. I put the stuff which was previously in the anonymous namespace into the internal namespace. Reasonable?

Makes sense. I'd only rename printf_formatter.h to just printf.h.

[4] I'd be inclined to try to split the two interfaces into two header files if you're amenable to that. In my opinion format.h could be split up a little.

Do you mean printf and Python-like formatting APIs? While I agree that its worth doing, I suggest addressing in a separate PR to keep changes manageable.

[5] Like with 235, I guess I should pull the ArgFormatterBase out internal

I'm not sure it's necessary. Users will probably need to subclass PrintfArgFormatter rather than ArgFormatterBase. The latter is more like an implementation detail.

[6] If I do 4, maybe we could make the naming of ArgFormatterBase more consistent with the naming of BasicFormatter?

Any suggestions to improve naming consistency are welcome.

[vitaut]

And thanks for working on this!

[spacemoose]

I renamed printf_formatter.h as suggested.

Regarding point 5, I have to apologize if I'm being a little dense, but I'm not sure I follow you:

To customize the BasicFormatter, I create a custom formatter by subclassing the BasicArgFormatter

For the PrintfArgFormatter, I modified ArgFormatterBase, as it seemed to fulfill the same role the PrintfArtfFormatter as the BasicArgFormatter does for BasicFormatter.

This allowed me to make an apparently successful test by defining the following custom formatter (you can see the full test in tests/custom-formatter.cc):

// A custom argument formatter that doesn't print `-` for floating-point values
// rounded to 0.
class CustomPAF : public fmt::internal::ArgFormatterBase<CustomPAF, char>
{
public:
  CustomPAF(fmt::BasicWriter<char> &w, fmt::FormatSpec &s):
    fmt::internal::ArgFormatterBase<CustomPAF, char>(w, s) {}

  void visit_double(double value) {
    if (round(value * pow(10, spec().precision())) == 0)
      value = 0;
    fmt::internal::ArgFormatterBase<CustomPAF, char>::visit_double(value);
  }
};

Which seems to work at least. Should I have been modifying PrintfArgFormatter instead/additionally?

[vitaut]

Printf argument formatting is implemented in PrintfArgFormatter and therefore I think you should inherit from it for customization purposes. ArgFormatterBase only implements formatting common between Python-like and printf argument formatting. You might need to add a template argument to PrintfArgFormatter to pass the visitor subclass like CustomPAF:

            +------------------+
            | ArgFormatterBase |
            +------------------+
                ^          ^
                |          |
+-------------------+  +--------------------+
| BasicArgFormatter |  | PrintfArgFormatter |
+-------------------+  +--------------------+
         ^                        ^
         |                        |
+-------------------+  +--------------------------+
| CusomArgFormatter |  | CustomPrintfArgFormatter |
+-------------------+  +--------------------------+

Hope it makes sense.

[spacemoose]

Thanks, your notes helped a great deal. I figured out my error, and made the necessary changes. I'll push a commit tomorrow.

[vitaut]

Great! Looking forward for a pull request =).

[spacemoose]

Sorry about the pollution with the pull requests. Is there a way to execute the tests without making a pull request? Esp with the windows build I don't have a way to test locally.

[vitaut]

I don't mind if you use a work-in-progress PR to check portability. I'm doing the same with branches as I don't have access to a Windows machine. BTW you can change a PR in place by modifying the original branch. Another option is to add your clone of the project to Travis/Appveyor.