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

Updating string formatting #504

Merged
merged 4 commits into from Jul 3, 2011
Merged

Conversation

takluyver
Copy link
Member

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.

@minrk
Copy link
Member

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.

@takluyver
Copy link
Member Author

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

@minrk
Copy link
Member

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.

@takluyver
Copy link
Member Author

Thanks, @minrk.

@minrk
Copy link
Member

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.

@takluyver
Copy link
Member Author

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.

@minrk
Copy link
Member

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.

@takluyver
Copy link
Member Author

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

@takluyver
Copy link
Member Author

Rebased after the merge of newapp.

@fperez
Copy link
Member

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...

@minrk
Copy link
Member

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.

@fperez
Copy link
Member

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.

@takluyver
Copy link
Member Author

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
@fperez
Copy link
Member

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.

@takluyver
Copy link
Member Author

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.

@fperez
Copy link
Member

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.

@takluyver
Copy link
Member Author

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.

@fperez
Copy link
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants