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.

[staujd02]

With Vim's help I removed the last couple linter problems

[martica]

After a closer look, it feels like adding the stubs without any behaviour isn't gaining a lot yet. They all currently have __format__ defined in their superclass, object, to just return the value for an empty format string, so defining stub methods doesn't provide any new functionality.

I think a more fruitful approach would be to implement __format__ for one type at a time and make it useful. A lot of the functionality that should be in the __format__ methods is in types/StrUtils.js and could be reused.

There is should be some decent examples in tests/datatypes/test_str.py for how to deal with formatting issues. You probably want to use the @transforms decorator on your tests or the current test infrastructure tries to reformat a number of things to aid with matching, but the output of format() should match without that. @awildtechno just wrote some docs on how to use that: https://github.com/beeware/batavia/blob/8c55a01a06687ee54fe5f5e37292571a3aa3c399/tests/utils/output_cleaners.py#L25

[martica]

If you want to chat a bit about how to best approach this, come join us on gitter (https://gitter.im/beeware/general).

[staujd02]

Thanks for the review @martica , and for informing me about the transforms decorator. During some exploratory testing I noticed that reformatting behavior (but was unaware on how to stop it).

If I fully implement the format option for the Bool type, would it be best to start a new pull request or would preserving the context of this pull request be preferable?

[martica]

You can start a new PR, or continue this one. Either way is fine.

Format has a lot of functionality. Don't feel like you have to completely implement Bool. Any additional functionality is valuable.

[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 notifications@github.com 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 https://github.com/beeware/batavia/pull/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.