burn python2.x bridges more #12948
burn python2.x bridges more #12948
Conversation
| print("X_test {}".format(X_test.shape)) | ||
| print("X_test.format = {}".format(X_test.format)) | ||
| print("X_test.dtype = {}".format(X_test.dtype)) | ||
| print("y_test {}".format(y_test.shape)) |
There was a problem hiding this comment.
All changes of the form
- "something {0}".format(x)
+ "something {}".format(x)
don't really bring anything and should be reverted IMO. It's one character less, for the cost of creating merge conflicts with all the open PRs.
There was a problem hiding this comment.
they do offer an improvement and were added in python2.7 -- numbering format specifiers is human-error-prone operation
There was a problem hiding this comment.
Yes and it's good to use them going forward, but adding merge conflicts for this seems not useful.
There was a problem hiding this comment.
I think you could say the same about using super() -- what's the hangup here?
There was a problem hiding this comment.
Fair, that would be an argument to keep the super calls the same. But I do feel the new way makes them more readable. The "hangup" is that every code change brings maintenance costs with it and we're trying to balance the benefit of making the change with that cost.
This is the reason why we discourage changes that are largely cosmetic, like applying pep8.
There was a problem hiding this comment.
I don't really have that strong an opinion, though...
| @@ -203,7 +203,7 @@ class _PGNMF(NMF): | |||
| def __init__(self, n_components=None, solver='pg', init=None, | |||
| tol=1e-4, max_iter=200, random_state=None, | |||
| alpha=0., l1_ratio=0., nls_max_iter=10): | |||
| super(_PGNMF, self).__init__( | |||
| super().__init__( | |||
| @@ -384,11 +384,11 @@ def __str__(self): | |||
| # INTERNAL ==================================================================== | |||
| def encode_string(s): | |||
| if _RE_QUOTE_CHARS.search(s): | |||
| return u"'%s'" % _RE_ESCAPE_CHARS.sub(r'\\', s) | |||
| return "'%s'" % _RE_ESCAPE_CHARS.sub(r'\\', s) | |||
There was a problem hiding this comment.
This is a vendored package. All changes to sklearn/externals/* need to be reverted (and if you wish submitted upstream to the corresponding package).
|
closing as "largely cosmetic" |
|
I guess it's a bit spitting hairs which changes are cosmetic and which are not. Unfortunately we don't really have clear guidelines or consensus on that and I get it if you don't want to wade through that. Maybe it would make sense to swallow the bitter pill in this case and just make all the changes? I don't know. |
|
@asottile I have not realized that you wrote pyupgrade: it is quite nice. Personally most of the changes in this PR look good to me (now that #12812 was merged) except for python2.7+ format specifiers. The latter is responsible for almost half of the changes and I just don't see the (debatable) advantages it brings in this case justifying the risk of merge conflicts. It's a bit along the same line as the argument about keeping percent formatting in asottile/pyupgrade#63 (comment) It works, the previous way is not deprecated, it's only slightly more compact and the fact that this was not supported in python2.6- is not reason enough to start using it. Any chance of an option in pyupdate to disable the python2.7+ format which could be applied to this PR? All the other changes look quite useful. |
|
The I don't have interest in participating in option-hell at personal preference whims -- the whole point of the tool and tools like it (black, import reorderers, whitespace trimmers, etc.) is that you don't discuss stylistic nitpicks: format the code automatically and focus on architectural changes. The reason to use py27+ format literals is not "because it breaks python2.6" and I've not suggested that. The reason to use it is the same reasons the syntax was introduced in the first place -- it's less error prone. If you're afraid of merge conflicts, I'd suggest adding more aggressive formatting tools instead of shying away from them. For instance add-trailing-comma or any of the other formatters / linters might be a good idea to invest in. |
In general, I adhere to that vision and am pretty enthusiastic about black (and pyupgrade). However, when you take a large legacy project like scikit-learn we currently have 680 open PRs. A large number of these PRs were made at a significant cost both in contributor and reviewers time, can take years (sic) to merge, and are still very relevant today. So we have to take into account the cost of resolving merge conflicts, auto-formatting tools only help so much in this situation.
Well, that's the feeling I got from the PR title "burn python2.x bridges more".
I'm not really interested in a long discussion about this, but after skimming through the issue where it was introduced https://bugs.python.org/issue5237 it seemed mostly about convenience. Presumably, you are not changing functionality there, so if any code was wrong, it would still be. Also, those same lines could use f-string formatting soon. Anyway, I have continued this in #12997 (can't re-open this PR since you deleted the branch). |
I saw https://twitter.com/amuellerml/status/1083129999643824128 and #12639 and was inspired
This was almost entirely automated via pyupgrade
I ran:
And then fixed the flake8 issues pointed out by