Add mixed line ending hook by nagromc · Pull Request #218 · pre-commit/pre-commit-hooks

nagromc · 2017-07-17T19:31:16Z

Closes #196

To make it easier to use in the program (single string instead of a list of strings)

Fixes #1

'--fix=cr' is not yet implemented though

asottile

Overall looks great! This'll be a great addition to the hooks 👍

The branch needs a rebase / pull (there's some conflicts) but overall the idea looks sound.

I've highlighted some small issues below

asottile · 2017-07-17T20:44:52Z

+    language: python
+    entry: mixed-line-ending
+    description: Replaces or checks mixed line ending
+    files: ''


(once you rebase) this'll be types: [text] instead (using the brand new in pre-commit 0.15.0 types)

asottile · 2017-07-17T20:46:01Z

+    - `--fix={auto,crlf,lf,no}`
+        - `auto` - Replaces automatically the most frequent line ending. This is the default argument.
+        - `crlf`, `lf` - Forces to replace line ending by respectively CRLF and LF.
+        - `no` - Checks if there is any mixed line ending.


I'd say something like: "Error on mixed line endings but do not automatically fix" myself (just so it's more clear that this changes the hook from a fixer into just a checker)

"Checks if there is any mixed line ending without modifying any file"?

asottile · 2017-07-17T20:47:14Z

+class LineEnding(Enum):
+    CR = b'\r', '\\r', 'cr', re.compile(b'\r(?!\n)', re.DOTALL)
+    CRLF = b'\r\n', '\\r\\n', 'crlf', re.compile(b'\r\n', re.DOTALL)
+    LF = b'\n', '\\n', 'lf', re.compile(b'(?<!\r)\n', re.DOTALL)


hmmm I wonder if there's a way that str_print can be done automatically with repr(...)

asottile · 2017-07-17T20:48:28Z

+
+class MixedLineDetection(Enum):
+    NOT_MIXED = 1, False, None
+    UNKNOWN = 2, False, None


ah, we could remove the equality hack if we do something simple like "lf wins on ties" or something. Just an idea

Well, isn't it platform-related? I mean, Windows users would not appreciate to have their files changed in lf file ending.

if they're 50/50 I think it's probably fine?

Actually, mixed_line_ending.py is able to detect LF, CRLF, and CR. So it would be 33% each.

that makes it even more rare, I'd say just pick one of them if there's ties (since it also simplifies other code elsewhere iirc)

Up to you though, this is fine as is :)

asottile · 2017-07-17T20:49:36Z

+    # or \r
+    b'|' + LineEnding.CR.regex.pattern +
+    b')'
+)


I think this constant here reads fine (if not better) without the comments -- the previous constants allow this expression to read nicely without

asottile · 2017-07-17T20:58:58Z

+        logging.info('The following files have mixed line endings:\n\t%s',
+                     '\n\t'.join(mle_filenames))
+
+    return 1 if mle_found else 0


aka return int(mle_found), though this is fine

asottile · 2017-07-17T20:59:43Z

+        if detect_result == MixedLineDetection.NOT_MIXED:
+            logging.info('The file %s has no mixed line ending', filename)
+
+            mle_found |= False


x |= False is a noop (can remove this line)

Completely true omg!

asottile · 2017-07-17T21:01:47Z

+            mle_found |= False
+        elif detect_result == MixedLineDetection.UNKNOWN:
+            logging.info('Could not define most frequent line ending in '
+                         'file %s. File skiped.', filename)


asottile · 2017-07-17T21:02:21Z

+
+
+def _convert_line_ending(filename, line_ending):
+    # read the file


can remove this comment, it doesn't aid in readability (generally stick to why comments and not what comments)

asottile · 2017-07-17T21:05:55Z

+import re
+import sys
+
+from enum import Enum


in python 2 we'll need to declare this as a dependency.

The magic to doing this is to add this to setup.py:

extras_require={':python_version=="2.7"': ['enum34']}

I actually made a video on this topic as well :D

Well I think I miss something. The tests pass with Python 2.7. But I haven't declared anything relative to enum. Yet, it seems to be "imported" with some magic.

Is it some kind of pre-built environment provided by tox?

Also, how can I install dependencies provided in extras_require? So far, it worked because I installed enum34 with sudo apt install python-enum34, which is not the universal way to do this FWIU.

Using requirements-tools's visualize-requirements (that I wrote as well, lol), it seems that enum34 is being pulled in implicitly from flake8:

# first comment out `-e .` $ visualize-requirements requirements-dev.txt coverage flake8 - pyflakes<1.6.0,>=1.5.0 - enum34 - configparser - mccabe<0.7.0,>=0.6.0 - pycodestyle>=2.0.0,<2.4.0 mock - six>=1.9 - funcsigs>=1 - pbr>=0.11 pre-commit - aspy.yaml - pyyaml - six - cached-property - pyyaml - identify>=1.0.0 - virtualenv - nodeenv>=0.11.1 pytest - setuptools - py>=1.4.33

The extras_require syntax above just tells (recent enough) pip that "for python2.7, I have these dependencies" -- pip will know what to do automatically.

nagromc · 2017-07-18T13:56:06Z

The branch needs a rebase / pull

Do you mean I'm in charge of the integration of this new hook, and I need to resolve the conflict? I'm not used to do pull request, so I don't know who has to do the job. Please tell me 😄

I'll fix soon the issues you pointed out.

asottile · 2017-07-18T16:48:53Z

The branch needs a rebase / pull

Do you mean I'm in charge of the integration of this new hook, and I need to resolve the conflict? I'm not used to do pull request, so I don't know who has to do the job. Please tell me 😄

Yep! On your branch run

git remote add upstream git@github.com:pre-commit/pre-commit-hooks
git fetch upstream
# either of the following two:
# this one is probably easier -- if you're not super comfortable with git go with this option -- it will preserve your current history and add a merge commit (where you'll resolve the conflicts)
git merge upstream/master
# this will replay your commits on top of master, many consider this more "clean" but it doesn't matter too much
git rebase -i upstream/master

nagromc · 2017-07-18T17:12:11Z

I'm quite comfortable with Git, no problem with that. Was just asking about who is usually in charge of the merges in your work flow.

Personally, I prefer to merge a branch to another instead of rebase. I rebase branches when I work on the same branch as others. But it is an endless debate 😃

asottile · 2017-07-18T17:14:41Z

👍 whatever you're comfortable with!

asottile · 2017-07-21T05:14:21Z

Let me know when you're ready for me to do another pass at review (maybe you're already ready?) -- there's been a lot of emails from this thread :)

asottile · 2017-09-06T03:24:38Z

we'll continue this in #233

nagromc added 30 commits June 13, 2017 21:40

Add mixed-line-ending test case

56d4543

Add mixed-line-ending processor

3d4fb41

Use Enum to list line ending types in mixed_line_ending

16b7c7a

Add enum to mixed_line_ending --fix option

afaa97c

Split argument parsing from main mixed line ending function

5186664

Change --fix option from mixed_line_ending

93194b9

To make it easier to use in the program (single string instead of a list of strings)

Use enum instead of raw argparse result

b2b0d59

Add filenames option

ad0062a

Add line ending detection

466f9e1

Add line ending conversion

aaf134c

Change files according to --fix option

0a8b929

Rename variable names

2b28f4f

Reuse variable definition

22b2282

Add logs to mixed_line_ending.py

f477582

Add unit test for mixed_line_ending

b1294b8

Refactor MixedLineDetection

4270b56

Add mixed line detection

a1ffbfa

Refactor mixed_line_ending

c6c4c4a

Fix _process_fix_auto to return the right value

614893f

Refactor mixed_line_ending

a1e1421

Improve logging for force line ending

609d011

Add unit test for mixed_line_ending --fix={cr,crlf}

63bb1fd

Improve test coverage

3dbeeee

Ignore .cache/

d0016c5

Refactor _process_no_fix

ba63d1b

Fix 13 tests for Python 3.4 & 3.5

2b6ad97

Fix the 5 remaining tests for Python 3.4 & 3.5

d16d04a

Fixes #1

Refactor file opening

8bc4af4

Refactor _detect_line_ending

4fc9624

Add support for CR line ending for mixed_line_ending.py

1937788

'--fix=cr' is not yet implemented though

nagromc added 2 commits July 10, 2017 22:33

Add mixed-line-ending hook declaration

560e1c2

Update README.md

0335ebf

asottile requested changes Jul 17, 2017

View reviewed changes

nagromc added 3 commits July 18, 2017 19:40

Merge remote-tracking branch 'upstream/master' into mixed-line-ending

55658c4

Clean up against add-trailing-comma

ab2a849

Use new features from pre-commit 0.15.0

41ff0e1

nagromc force-pushed the mixed-line-ending branch from 95d161e to 41ff0e1 Compare July 18, 2017 18:29

nagromc added 8 commits July 18, 2017 21:47

Update README.md based on maintainer's wish

eb0c3ba

Add Pyhton 2.7 'enum34' dependency declaration

f795097

Refactor pre_commit_hooks/mixed_line_ending.py

f58b552

Remove non relevant comments and make others more explicit

4be276c

Refactor pre_commit_hooks/mixed_line_ending.py

ef4a323

Remove -v/--verbose option on mixed_line_ending.py

4d3d8e1

Remove _check_filenames in mixed_line_ending.py

f9915cb

Refactor mixed_line_ending.py

0e223bc

asottile mentioned this pull request Sep 6, 2017

Add mixed-line-ending hook #233

Merged

asottile closed this Sep 6, 2017



		def _convert_line_ending(filename, line_ending):
		# read the file

Uh oh!

Conversation

nagromc commented Jul 17, 2017

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagromc commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Jul 18, 2017

Uh oh!

nagromc commented Jul 18, 2017

Uh oh!

asottile commented Jul 18, 2017

Uh oh!

asottile commented Jul 21, 2017

Uh oh!

asottile commented Sep 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

nagromc commented Jul 18, 2017 •

edited

Loading