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

Custom FormatSpec (#439) #444

Merged
merged 7 commits into from
Dec 30, 2016
Merged

Custom FormatSpec (#439) #444

merged 7 commits into from
Dec 30, 2016

Conversation

polyvertex
Copy link
Contributor

No description provided.

@polyvertex
Copy link
Contributor Author

Tested on VS2015 only... Obviously.

@vitaut
Copy link
Contributor

vitaut commented Dec 30, 2016

Thanks for the PR. Mostly looks good but could you default the Spec format argument to FormatSpec and remove explicit passing of this argument wherever possible to make it a bit cleaner?

@polyvertex
Copy link
Contributor Author

Clean enough?

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor comments re formatting.

@@ -120,7 +120,7 @@ custom argument formatter class::
// A custom argument formatter that formats negative integers as unsigned
// with the ``x`` format specifier.
class CustomArgFormatter :
public fmt::BasicArgFormatter<CustomArgFormatter, char> {
public fmt::BasicArgFormatter<CustomArgFormatter, char> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought it was just a mistake. Why the extra space?

Copy link
Contributor

Choose a reason for hiding this comment

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

LOL, my bad. I misread the diff and thought it was added. No need for an extra space of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(git is so much fun)

class CustomArgFormatter
: public fmt::BasicArgFormatter<CustomArgFormatter, char> {
class CustomArgFormatter :
public fmt::BasicArgFormatter<CustomArgFormatter, char> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the original formatting here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I have changed this one to comply to the one below, which I left untouched from master:

class CustomPrintfArgFormatter :
    public BasicPrintfArgFormatter<CustomPrintfArgFormatter, char> {

I'll correct both then

@polyvertex
Copy link
Contributor Author

Style changes applied. It got quite noisy here so I can create an all-in-one PR once you've validated the whole thing if you prefer.

@vitaut vitaut merged commit 554946a into fmtlib:master Dec 30, 2016
vitaut pushed a commit that referenced this pull request Dec 30, 2016
* A custom FormatSpec type can be passed as a template argument to the ArgFormatter chain (#439)

* Corrected nested-name-specifier error

* Spec template argument defaulted to FormatSpec

* Forward declaration of FormatSpec

* Style

* Style (part 2)

* Style (part 3)
@vitaut
Copy link
Contributor

vitaut commented Dec 30, 2016

No worries, I squashed it via GitHub interface. Thanks for contributing!

@polyvertex polyvertex deleted the issue439 branch January 1, 2017 14:07
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.

2 participants