Extend Builtin Format function

[staujd02]

Format was implemented into types where format(, "") is the only accepted string format specifier

Format can now be implemented on more types.

PR Checklist:

• [ x] All new features have been tested
• [ x] All new features have been documented
• [ x] I have read the CONTRIBUTING.md file
• [ x] I will abide by the code of conduct

[martica]

Howdy @staujd02. Thanks for working on this!

The eslint check in CI is broken currently, so I'm stuck enforcing it manually at the moment. Can you replace the template strings with concatenation? Our .eslintrc currently doesn't allow them. :( You can refer to it in the top level of the repo.

[staujd02]

Sure thing @martica, Done! Took care of the auto linter too...

[martica]

I'll take a closer soon, but moving to calling the __format__ methods on types is a great step in the right direction here.

[martica]

Looks like there are a couple more errors that Brutus is grumbling about.

[alexjdw]

Looking at this briefly, I think most of the functionality already exists for you in Str.format. Could you do something like this?

s = "{" + formatSpecifier + "}"
return callables.call_method(s, 'format', [this], {}) # Batavia equivalent of s.format(this)

Does that make sense as a boilerplate?

[staujd02]

Yep, that makes sense.

[martica]

We do need to keep in mind that eventually str.format should be calling __format__ on the types that are being formatted. For now, having the various __format__ methods call StrUtils._new_subsitute would get a lot of reuse quickly.

For a long-term goal, it feels like we need to find a seem in StrUtils._new_subsitute between its parsing of the full format strings and its formatting of individual values and __format__ in the types could call into the latter part.

[staujd02]

I'm trying to redirect the formatting to string.format() and in effect StringUtils.__new_substitute(). But the arguments are being converted into a string types and therefore type conversions valid for the Boolean type are being rejected since they don't apply to strings...

[martica]

That sounds frustrating. I can try to help this evening (PDT) if you are still stuck.

[alexjdw]

@martica so if I'm understanding right, the end result should actually be Str.format calls something like <type>.__format__(arg, format_style) for each substitution? That's going to be a very, very thick commit as the current format is about 1500 lines of switch/case choose your own adventure.

[martica]

That's the way cpython implements it, or at least makes it appear that way. If you implement `__format__` on a custom type string.format will call it. We don't have to follow the exact implementation, but does make some things easier. That nest of switches is something that would be much more clearly represented with polymorphism.

…

On Fri, Jun 14, 2019 at 4:06 PM awildtechno ***@***.***> wrote: @martica <https://github.com/martica> so if I'm understanding right, the end result should actually be Str.format calls something like .*format*(arg, format_style) for each substitution? That's going to be a very, very thick commit as the current format is about 1500 lines of switch/case choose your own adventure. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#816?email_source=notifications&email_token=AAAUWIJI23245APXXJEU7N3P2QQA5A5CNFSM4HXR7OUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXYJNEQ#issuecomment-502306450>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUWINIZIZBC2RCY7TKPLDP2QQA5ANCNFSM4HXR7OUA> .

[martica]

I took a look @staujd02. It looks like the problem comes in here: https://github.com/beeware/batavia/blob/master/batavia/types/StrUtils.js#L1173

Bool is actually an outlier in that if you ask for string formatting, it shows the english word, but any numeric formatting treats it as 0 or 1. Seems like that chain of ifs needs a case for Bool, and because it depends on the format type (empty string or 's' vs. all the other types) you'll need to move the format parsing before the arg gets set. Look at https://github.com/beeware/batavia/blob/master/batavia/types/StrUtils.js#L796

Fixing the current string.format() is a major refactoring. Don't worry about that. Having the format builtin call string.format directly should get us a lot without making the future refactor any harder.

[staujd02]

@martica that sounds like a plan. I'll bump the PR if something else crops up.

[staujd02]

Thanks for the help @martica and @awildtechno ! Hope this is close to what you are looking for...

[alexjdw]

I'm not really qualified to review but I wanted to say Thanks for sticking it out with this. I just got started here a few weeks ago with some modifications to Str.format and I know how nitpicky writing and passing tests for the formatter can be.