Updating string formatting #504

Merged
merged 4 commits into from Jul 3, 2011

Conversation

Projects
None yet
3 participants
Owner

takluyver commented Jun 6, 2011

This replaces most of the uses of the old Itpl library with built-in string formatting (fancy string formatting was new in Python 2.6). I'd like to also tidy up the remaining cases (prompt generation and magic/system calls), but these are trickier, so are probably best done separately.

Owner

minrk commented Jun 6, 2011

This sounds good to me, but your commits in the parallel code will almost certainly conflict with #486. Hopefully we can get the newapp code merged quickly, so this stops happening and code can merge cleanly.

Owner

takluyver commented Jun 6, 2011

I had a feeling they probably would. I'll rebase once that's all merged.

Owner

minrk commented Jun 9, 2011

@takluyver - I'll go ahead and just make the changes in the parallel code, so you don't have to worry about the conflict.

Owner

takluyver commented Jun 10, 2011

Thanks, @minrk.

Owner

minrk commented Jun 11, 2011

I did discover when updating the docs that there is a disadvantage to using string formatting vs Itpl: Itpl supports expressions. This is actually quite useful in batch script templates, where you might have a line like #PBS -l nodes=${n/4}:ppn=4.

I've already made the change in the parallel code, and I just now remembered the docs, discovering this while updating them. Having now remembered its value, I think I will actually revert to Itpl for the batch templates.

Owner

takluyver commented Jun 11, 2011

I think it should be possible to subclass Formatter so that it can evaluate arbitrary expressions - that way we let Python itself handle parsing the string.

Owner

minrk commented Jun 11, 2011

Thanks for the pointer. Turns out:

 class EvalFormatter(Formatter):
    """Formatter with support for expressions"""
   def get_value(self, key, args, kwargs):
       if isinstance(key, (int, long)):
           return args[key]
       else:
           return eval(key, kwargs)

should cover it.

Owner

takluyver commented Jun 11, 2011

Yes, that was roughly what I had in mind. Excellent.

Owner

takluyver commented Jun 21, 2011

Rebased after the merge of newapp.

Owner

fperez commented Jul 3, 2011

Any reason why we shouldn't merge this too before cutting the RC? The more we move towards the standard python mechanisms and away from homebrew solutions like itpl, the better...

Owner

minrk commented Jul 3, 2011

I think the only reason not to is that itpl is still used in a few places, and we could finish the transition at once.

Owner

fperez commented Jul 3, 2011

I suspect the full removal of itpl will take some extra work/time, because it's heavily used in prompt generation. And that's one area where we have some regressions that will take a bit of time to sort out. So I favor merging this now so the code doesn't bitrot, and tackling the remainder later on, when we bite the bullet on our prompt handling/generation mechanisms.

Owner

takluyver commented Jul 3, 2011

I've got a separate pull request which takes it out of prompt generation, but I don't think that will make it in before 0.11 (it's a substantial reworking of the prompt system). However I think the variable expansions for magic/system calls (!touch $filename) is a bit trickier to replace, unless we're happy to ditch the $ notation in favour of {} notation.

@takluyver takluyver merged commit e78a807 into ipython:master Jul 3, 2011

Owner

fperez commented Jul 3, 2011

Good point: we may need to keep itpl around for the $var notation. That is simply too convenient, easy to remember and entrenched during interactive shell-like usage. But the rest of our infrastructure can be more standards-compliant, just leaving itpl to support this (admittedly very nice and useful) interactive feature.

Owner

takluyver commented Jul 3, 2011

Merged.

In terms of the $var notation, we could easily subclass Formatter to pick up variables like that with a dash of regex. However, itpl can currently handle arbitrary Python expressions, and there's no way to write a regex that can handle it all. Literally no way, from what I understand - it requires a turing complete language to parse properly. Limited to object names, it could work.

Owner

fperez commented Jul 3, 2011

On Sun, Jul 3, 2011 at 1:33 PM, takluyver
reply@reply.github.com
wrote:

Merged.

Great, thanks!

In terms of the $var notation, we could easily subclass Formatter to pick up variables like that with a dash of regex. However, itpl can currently handle arbitrary Python expressions, and there's no way to write a regex that can handle it all. Literally no way, from what I understand - it requires a turing complete language to parse properly. Limited to object names, it could work.

Well, we could demand that anything beyond $foo or $foo.bar needs
to be braced as ${x+y[0]}. Then a regexp approach can probably
work, since it only needs to catch those few cases: unbraced
alpha+dots, or braced anything.

I'm not sure if that effort is worth it for dumping itpl, though: itpl
is simple and does the job we need fine, so I'm not majorly motivated
to completely rid ourselves of it when the last part of the job will
require new custom code.

Owner

takluyver commented Jul 3, 2011

On the other hand, we could fairly easily drop a couple of hundred lines of code, and probably improve performance when it is used, because we're not parsing the string character by character in Python.

Owner

fperez commented Jul 3, 2011

On Sun, Jul 3, 2011 at 1:57 PM, takluyver
reply@reply.github.com
wrote:

On the other hand, we could fairly easily drop a couple of hundred lines of code, and probably improve performance when it is used, because we're not parsing the string character by character in Python.

Sure, if you'd like to take a crack at it post 0.11, I'm not opposed
at all. I was just thinking out loud about the cost/benefit ratio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment