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

CLN: replace %s syntax with .format in pandas.io #24454

Closed
wants to merge 9 commits into from

Conversation

makeajourney
Copy link
Contributor

progress towards #16130

  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use named parameters rather than positional, esp where there are more string to replace

@@ -2236,10 +2239,11 @@ def __init__(self, f, **kwds):
raise ValueError('Only length-1 decimal markers supported')

if self.thousands is None:
self.nonnum = re.compile('[^-^0-9^%s]+' % self.decimal)
self.nonnum = re.compile(
'[^-^0-9^{decimal}]+'.format(decimal=self.decimal))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make both of the raw string (r'....')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add this

"value->%s,format->%s,append->%s,kwargs->%s]"
% (t, group, type(value), format, append, kwargs)
)
"cannot properly create the storer for: [{}] [group->{},"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use named parameters here

@@ -1590,7 +1594,7 @@ def __unicode__(self):
self.axis,
self.pos,
self.kind)))
return "name->%s,cname->%s,axis->%s,pos->%s,kind->%s" % temp
return "name->{},cname->{},axis->{},pos->{},kind->{}".format(*temp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here as well, maybe better to write as
','.join(("[{}->{}".format(key, value) for key, value in zip(['name', 'cname', 'axis', 'pos', 'kind'], temp)

@jreback jreback added the Code Style Code style, linting, code_checks label Dec 27, 2018
@codecov
Copy link

codecov bot commented Dec 27, 2018

Codecov Report

Merging #24454 into master will decrease coverage by 49.3%.
The diff coverage is 64.19%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24454       +/-   ##
===========================================
- Coverage   92.31%      43%   -49.31%     
===========================================
  Files         163      163               
  Lines       51987    51987               
===========================================
- Hits        47990    22356    -25634     
- Misses       3997    29631    +25634
Flag Coverage Δ
#multiple ?
#single 43% <64.19%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/io/packers.py 14.82% <0%> (-73.26%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.17%) ⬇️
pandas/io/parsers.py 48.51% <5.88%> (-46.87%) ⬇️
pandas/io/pytables.py 91.38% <85%> (-0.93%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/core/categorical.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-98.65%) ⬇️
... and 125 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3086e0a...153ae0d. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 27, 2018

Codecov Report

Merging #24454 into master will increase coverage by <.01%.
The diff coverage is 84.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24454      +/-   ##
==========================================
+ Coverage   92.38%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52321    52321              
==========================================
+ Hits        48337    48338       +1     
+ Misses       3984     3983       -1
Flag Coverage Δ
#multiple 90.8% <39.28%> (ø) ⬆️
#single 43.06% <65.47%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/packers.py 88.21% <0%> (ø) ⬆️
pandas/io/sas/sas7bdat.py 91.16% <33.33%> (ø) ⬆️
pandas/io/pytables.py 92.31% <85.71%> (ø) ⬆️
pandas/io/parsers.py 95.4% <94.11%> (ø) ⬆️
pandas/util/testing.py 88.09% <0%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d46d2a5...f249d93. Read the comment docs.

"Consider using min_itemsize to preset the sizes on "
"these columns" % (itemsize, self.cname, c.itemsize))
"these columns".format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

many locations still that need to use kwargs

@@ -1840,7 +1849,7 @@ def create_for_block(
""" return a new datacol with the block i """

if cname is None:
cname = name or 'values_block_%d' % i
cname = name or 'values_block_{idx}'.format(idx=i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for a single replacement ok with NOT using kwargs

@@ -1876,7 +1885,8 @@ def __unicode__(self):
self.dtype,
self.kind,
self.shape)))
return "name->%s,cname->%s,dtype->%s,kind->%s,shape->%s" % temp
return ("name->{},cname->{},dtype->{},kind->{},"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the same template as above

@jreback
Copy link
Contributor

jreback commented Dec 28, 2018

let's be consistent about this. a single formatted string need not be named, multiple need to be named.

else:
col = '%s.%d' % (col, cur_count)
col = '{col}.{cnt}'.format(col=col, cnt=cur_count)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to use variable names consistently across modules, so count instead of int and column instead of col, etc...

@@ -1571,8 +1572,8 @@ def _get_name(icol):
return icol

if col_names is None:
raise ValueError(('Must supply column order to use %s as '
'index') % str(icol))
raise ValueError(('Must supply column order to use {icol!s} '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be explicit about type in this and subsequent example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you said about '!s', yes. !s is replacing the str function not '%s'.

raise ValueError("Unable to convert column %s to "
"type %s" % (column, cast_type))
raise ValueError(
"Unable to convert column {column} to type {type}".format(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does using type here present any potential conflict with the built-in? Maybe renamed to type_ to err on the side of caution?

@jbrockmendel
Copy link
Member

FWIW in case we someday want to lint for this, searching for "(?<!\d)\s%(?=\s)(?! \d)" shows 768 matches across the codebase. That pattern doesn't get rid of all the mod ops, but cuts it down quite a bit.

@WillAyd
Copy link
Member

WillAyd commented Jan 9, 2019

@makeajourney can you merge master and update comments?

@pep8speaks
Copy link

pep8speaks commented Jan 10, 2019

Hello @makeajourney! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 11, 2019 at 02:13 Hours UTC

@makeajourney
Copy link
Contributor Author

CI failed only with python2.7. I have no idea about this problem.

@makeajourney
Copy link
Contributor Author

@jreback Can I divide this pull-request into smaller units? It's too hard to find the problem because too many files changed.

@jreback
Copy link
Contributor

jreback commented Jan 11, 2019

yes that would be best

@makeajourney makeajourney deleted the issue-16130-io branch January 11, 2019 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants