MNT Run dos2unix #21053

DimitriPapadopoulos · 2021-09-15T12:10:13Z

What does this implement/fix? Explain your changes.

Run dos2unix on 4 files with CR LF Windows line endings instead of LF Unix line endings.

My initial motivation was that some diff tools show the additional CR as an annoying bold and red "^M"!

Any other comments?

Initially part of #21051.

jjerphan

Hi @DimitriPapadopoulos,

Thanks for unifying line endings, this sounds reasonable.

However contribution changes can't be integrated if there are linting errors, which is de facto to the case here.

The pre-commit setup should solve some of the problem. You can trigger it by calling black and by then committing changes.

DimitriPapadopoulos · 2021-09-30T13:25:46Z

i'll run black into a different commit, but same PR. Should fix the problem.

sklearn/svm/_newrand.pyx

jjerphan

This LGTM after removing some dead code. Thanks @DimitriPapadopoulos.

doc/tutorial/machine_learning_map/parse_path.py

doc/tutorial/machine_learning_map/pyparsing.py

jjerphan · 2021-10-06T08:34:35Z

doc/tutorial/machine_learning_map/pyparsing.py

+# ~ 'decorator to trim function calls to match the arity of the target'
+# ~ def _trim_arity(func, maxargs=3):
+# ~ if func in singleArgBuiltins:
+# ~ return lambda s,l,t: func(t)
+# ~ limit = 0
+# ~ foundArity = False
+# ~ def wrapper(*args):
+# ~ nonlocal limit,foundArity
+# ~ while 1:
+# ~ try:
+# ~ ret = func(*args[limit:])
+# ~ foundArity = True
+# ~ return ret
+# ~ except TypeError:
+# ~ if limit == maxargs or foundArity:
+# ~ raise
+# ~ limit += 1
+# ~ continue
+# ~ return wrapper


This can be removed.

Not sure about this one:

The function that is commented out is for Python 3:

scikit-learn/doc/tutorial/machine_learning_map/pyparsing.py

Lines 994 to 996 in d3429c1

# Only works on Python 3.x - nonlocal is toxic to Python 2 installs

#~ 'decorator to trim function calls to match the arity of the target'

#~ def _trim_arity(func, maxargs=3):

The current function is for Python 2-3:

scikit-learn/doc/tutorial/machine_learning_map/pyparsing.py

Lines 1015 to 1017 in d3429c1

# this version is Python 2.x-3.x cross-compatible

'decorator to trim function calls to match the arity of the target'

def _trim_arity(func, maxargs=2):

Shouldn't we discard the current Python 2-3 code and enable the commented out Python 3 code?

I feel this should be handled in a different PR.

That's a good question. Even though Python 2 is not supported anymore, a conservative choice would be to keep the current one if it works for Python 3. But the commented one used for Python 3 is way smaller and using it this would ease maintenance.

In any case, I am not sure if tutorial are referenced nor used anymore.

Cc: @glemaitre for a more authoritative point of view.

jjerphan · 2021-10-07T08:01:48Z

Please @DimitriPapadopoulos, do not force-push if your PRs got one or more review as reviews have to be done again entirely. Also, from a git log point of view, this won't have an impact because commits are squashed.

thomasjpfan

Thanks for the PR @DimitriPapadopoulos !

Since doc/tutorial/machine_learning_map/pyparsing.py and doc/tutorial/machine_learning_map/parse_path.py look vendored, I suggest not to change them.

I think we can keep the change in doc/tutorial/machine_learning_map/svg2imagemap.py and sklearn/svm/_newrand.pyx.

A handful files had CRLF Windows line endings. Change to LF Unix line endings for consistency, and because some diff tools show the additional CR as an annoying bold and red "^M"!

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

DimitriPapadopoulos · 2022-02-10T13:48:11Z

Ah, I hadn't noticed these two files are vendored files. I've removed these files from the commits.

thomasjpfan

LGTM

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

github-actions bot added cython module:svm labels Sep 15, 2021

DimitriPapadopoulos force-pushed the dos2unix branch 5 times, most recently from 8ce87b4 to 7182c26 Compare September 21, 2021 16:53

DimitriPapadopoulos force-pushed the dos2unix branch 3 times, most recently from b17ec7c to eade9d1 Compare September 27, 2021 09:30

DimitriPapadopoulos force-pushed the dos2unix branch from eade9d1 to ad218f5 Compare September 30, 2021 08:17

jjerphan self-requested a review September 30, 2021 11:29

jjerphan requested changes Sep 30, 2021

View reviewed changes

DimitriPapadopoulos force-pushed the dos2unix branch 2 times, most recently from 4cef309 to 8b979c1 Compare September 30, 2021 13:36

jjerphan reviewed Sep 30, 2021

View reviewed changes

sklearn/svm/_newrand.pyx Outdated Show resolved Hide resolved

DimitriPapadopoulos force-pushed the dos2unix branch 4 times, most recently from 053b03e to 9c9090f Compare October 5, 2021 21:56

jjerphan approved these changes Oct 6, 2021

View reviewed changes

DimitriPapadopoulos force-pushed the dos2unix branch 4 times, most recently from ead6ffa to 502ca37 Compare October 6, 2021 18:27

thomasjpfan reviewed Feb 9, 2022

View reviewed changes

DimitriPapadopoulos and others added 2 commits February 10, 2022 14:45

dos2unix

a8d3c69

A handful files had CRLF Windows line endings. Change to LF Unix line endings for consistency, and because some diff tools show the additional CR as an annoying bold and red "^M"!

Run flake8/black after dos2unix

269adb8

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

DimitriPapadopoulos force-pushed the dos2unix branch from 502ca37 to 269adb8 Compare February 10, 2022 13:46

thomasjpfan changed the title ~~dos2unix~~ MNT Run dos2unix Feb 10, 2022

thomasjpfan approved these changes Feb 10, 2022

View reviewed changes

thomasjpfan merged commit 84bc634 into scikit-learn:main Feb 10, 2022

DimitriPapadopoulos deleted the dos2unix branch February 11, 2022 18:18

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Mar 1, 2022

MNT Run dos2unix (scikit-learn#21053)

646a7f4

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT Run dos2unix #21053

MNT Run dos2unix #21053

DimitriPapadopoulos commented Sep 15, 2021 •

edited

Loading

jjerphan left a comment

DimitriPapadopoulos commented Sep 30, 2021

jjerphan left a comment

jjerphan Oct 6, 2021

DimitriPapadopoulos Oct 6, 2021

DimitriPapadopoulos Oct 6, 2021

jjerphan Oct 6, 2021

jjerphan commented Oct 7, 2021

thomasjpfan left a comment

DimitriPapadopoulos commented Feb 10, 2022

thomasjpfan left a comment

	# Only works on Python 3.x - nonlocal is toxic to Python 2 installs
	#~ 'decorator to trim function calls to match the arity of the target'
	#~ def _trim_arity(func, maxargs=3):

	# this version is Python 2.x-3.x cross-compatible
	'decorator to trim function calls to match the arity of the target'
	def _trim_arity(func, maxargs=2):

MNT Run dos2unix #21053

MNT Run dos2unix #21053

Conversation

DimitriPapadopoulos commented Sep 15, 2021 • edited Loading

What does this implement/fix? Explain your changes.

Any other comments?

jjerphan left a comment

Choose a reason for hiding this comment

DimitriPapadopoulos commented Sep 30, 2021

jjerphan left a comment

Choose a reason for hiding this comment

jjerphan Oct 6, 2021

Choose a reason for hiding this comment

DimitriPapadopoulos Oct 6, 2021

Choose a reason for hiding this comment

DimitriPapadopoulos Oct 6, 2021

Choose a reason for hiding this comment

jjerphan Oct 6, 2021

Choose a reason for hiding this comment

jjerphan commented Oct 7, 2021

thomasjpfan left a comment

Choose a reason for hiding this comment

DimitriPapadopoulos commented Feb 10, 2022

thomasjpfan left a comment

Choose a reason for hiding this comment

DimitriPapadopoulos commented Sep 15, 2021 •

edited

Loading