Fix "Prefer format() over string interpolation operator" issue

[orbeckst]

This pull request automatically fixes all occurrences of the following issue:

Issue type: Prefer format() over string interpolation operator
Issue details: https://www.quantifiedcode.com/app/project/gh:MDAnalysis:mdanalysis?groups=code_patterns/%3A4ACGxFj1

To adjust the commit message or the actual code changes, just rebase or cherry-pick the commits.
For questions or feedback reach out to [email protected].

Legal note: We won't claim any copyrights on the code changes.

Cheers,
Cody - Your code quality bot

[orbeckst]

This would have been one of the most tedious refactorings I could imagine... (especially as I never remember the exact format mini language). @MDAnalysis/coredevs , do you like this or is there a reason to stick with old interpolated strings?

[jbarnoud]

Cody is sometime overdoing it. Most of the time, it is not necessary to specify a format; '{0:s}'.format(foo) can be written '{0}'.format(foo), for instance.

[orbeckst]

Is the "overdoing" enough to not merge?
Should the auto-fix be changed (not sure if this is possible) to be less aggressive with explicit typing?

[kain88-de]

The overdoing doesn't hurt. The overdoing ensures that everythings works exactly as before. So being a little bit over verbose is quite ok I'd say

[orbeckst]

Need to check why XYZReader and friends fail with format strings.

I think we can push to this branch autofix/wrapped2_to3_fix-3 to update the PR.

[orbeckst]

Why were these exclamation marks inserted by @quantifiedcode-bot ? The py 2.7 specs of the format mini language don't contain ! as far as I can tell.

[kain88-de]

@orbeckst Look one paragraph above

The field_name is optionally followed by a conversion field, which is preceded by an exclamation point '!'

[jbarnoud]

I did not know about the exclamation mark either. Yet, the doc does mention it:

In less formal terms, the replacement field can start with a field_name that specifies the object whose value is to be formatted and inserted into the output instead of the replacement field. The field_name is optionally followed by a conversion field, which is preceded by an exclamation point '!', and a format_spec, which is preceded by a colon ':'. These specify a non-default format for the replacement value.

[jbarnoud]

Ah ! @kain88-de is faster than I am by 40 seconds !

[orbeckst]

@kain88-de --- thanks! Learnt something new .. such as RTFM...)

Why did it fail in some cases, though? And more importantly: are there more such cases?

@jbarnoud pointed out that the PR now passing does not mean that there aren't any other stinkers in this PR --- we don't cover many of the warning messages through the tests. A quick look at the diff showed that there were many more exclamation marks inserted.

[kain88-de]

It might be a good idea to ask the people from QuantifiedCode.

[programmdesign]

@kain88-de @orbeckst Happy to help. Can you explain what "failed in some cases"?

[jbarnoud]

@programmdesign
Two of cody changes lead to exceptions that were not raised previous to the changes.

First problematic change

The first change goes from:

logger.error("%4s %3d %3s %3s %6.3f  |  %4s %3d %3s %3s %6.3f" %
                    (ar.segid, ar.resid, ar.resname, ar.name, ar.mass,
                     at.segid, at.resid, at.resname, at.name, at.mass,))

to

logger.error("{0:4!s} {1:3d} {2:3!s} {3:3!s} {4:6.3f}  |  {5:4!s} {6:3d} {7:3!s} {8:3!s} {9:6.3f}".format(ar.segid, ar.resid, ar.resname, ar.name, ar.mass,
                          at.segid, at.resid, at.resname, at.name, at.mass))

where *.segid is a string, so as *.name and *.resname; *.resid is an integer, and *.mass is a float.

The error is:

ValueError: Invalid conversion specification

It is solved by removing the exclamation marks, which gives:

logger.error("{0:4s} {1:3d} {2:3s} {3:3s} {4:6.3f}  |  {5:4s} {6:3d} {7:3s} {8:3s} {9:6.3f}".format(ar.segid, ar.resid, ar.resname, ar.name, ar.mass,
                           at.segid, at.resid, at.resname, at.name, at.mass))

Second problematic change

The second change narrows the issue:

self._xyz.write("%8s  %10.5f %10.5f %10.5f\n" % (atom, x, y, z))

becomes

self._xyz.write("{0:8!s}  {1:10.5f} {2:10.5f} {3:10.5f}\n".format(atom, x, y, z))

which causes the same exception as for the first change and get solved in the same way.

Rational

I played a bit in iPython and it appears that '{0:3!s}'.format('a') raises the exception we are seing. On the contrary '{0!s:3}'.format('a') is fine. Note however that both '{0!s:3}'.format('a') and '{0:3s}'.format('a') are different from '%3s' % 'a'. Indeed:

In [13]: '%3s' % 'a'
Out[13]: '  a'

In [14]: '{0!s:3}'.format('a')
Out[14]: 'a  '

In [15]: '{0:3s}'.format('a')
Out[15]: 'a  '

The correct replacements for '%3s' % 'a' are:

In [16]: '%3s' % 'a'
Out[16]: '  a'

In [17]: '{0!s:>3}'.format('a')
Out[17]: '  a'

In [18]: '{0:>3s}'.format('a')
Out[18]: '  a'

So my quick fix does not actually fix the problem as the format output is not the expected one...

Finally, and this is mostly for MDAnalysis devs as it is likely difficult to find automatically, the first change could be made even clearer with the format mini language:

logger.error("{ar.segid:>4s} {ar.resid:3d} {ar.resname:>3s} {ar.name:>3s} {ar.mass:6.3f}  |  {at.segid:>4s} {at.resid:3d} {at.resname:>3s} {at.name:>3s} {at.mass:6.3f}".format(ar=ar, at=at))

[orbeckst]

@jbarnoud thanks for the detailed analysis. Just one question: in the first Cody example in First problematic change I don't see any exclamation marks.

[jbarnoud]

@orbeckst My bad, I copied the wrong version. I edited my comment accordingly.

[orbeckst]

@jbarnoud , from what I understand, the real fix should look like '{0:>3s}'.format('a') or actually, {ar.segid:>4s} or similar. Your fix 39c969e does not implement these changes.

Is it worthwhile continuing with this PR (and properly fixing all occurrences) or should we wait until we have better test coverage #597 and/or have taught cody the replacement that we would prefer? I am actually not to bothered about the exact formatting in output strings but when writing files with defined formats then we have to be careful.

[jbarnoud]

On 19/12/15 22:02, Oliver Beckstein wrote:

@jbarnoud https://github.com/jbarnoud , from what I understand, the
real fix should look like |'{0:>3s}'.format('a')| or actually,
|{ar.segid:>4s}| or similar. Your fix 39c969e
39c969e
does not implement these changes.

Is it worthwhile continuing with this PR (and properly fixing all
occurrences) or should we wait until we have better test coverage #597
#597 and/or have
taught cody the replacement that we would prefer? I am actually not to
bothered about the exact formatting in output strings but when writing
files with defined formats then we have to be careful.

—
Reply to this email directly or view it on GitHub
#592 (comment).

My fix make the test pass but is not correct. I can look at fixing all
the problematic changes made by cody, but a most of them are not tested.
Fixing #597 would make a big difference here as many cody's replacement
are in exception messages.

[orbeckst]

On 19 Dec, 2015, at 16:08, Jonathan Barnoud wrote:

My fix make the test pass but is not correct. I can look at fixing all
the problematic changes made by cody,

Fixing them "the right way"?

but a most of them are not tested.
Fixing #597 would make a big difference here as many cody's replacement
are in exception messages.

As long as we know that the conversion will always give a string, I don't overly care about the exact alignment of the inserted string. However, anything that writes to a file is critical and would need to be checked, i.e. have a test case that passes before and after.

Is it worthwhile?

[jbarnoud]

On 19/12/15 22:11, Oliver Beckstein wrote:

On 19 Dec, 2015, at 16:08, Jonathan Barnoud wrote:

My fix make the test pass but is not correct. I can look at fixing all
the problematic changes made by cody,

Fixing them "the right way"?
I can have a look after the holidays.

but a most of them are not tested.
Fixing #597 would make a big difference here as many cody's replacement
are in exception messages.

As long as we know that the conversion will always give a string, I
don't overly care about the exact alignment of the inserted string.
However, anything that writes to a file is critical and would need to
be checked, i.e. have a test case that passes before and after.

Is it worthwhile?
The tests should just trigger the exception. The message does not matter
that much. In the context of this issue, it would make sure that the
string format execute without issue. More generally, we assume that bad
input raises a given exception, but does it?

—
Reply to this email directly or view it on GitHub
#592 (comment).

[kain88-de]

@jbarnoud any news on this?

[jbarnoud]

On 12/01/16 23:08, kain88-de wrote:

@jbarnoud https://github.com/jbarnoud any news on this?

—
Reply to this email directly or view it on GitHub
#592 (comment).

I am rather busy this week, but I'll try to have that done before the
week-end.