Subject: Re: [quicwg/base-drafts] Python linting script (#1032)

From: Martin Thomson <[email-masked]>
Date: Mon, 01 Jan 2018 23:17:51 +0000 (UTC)

martinthomson commented on this pull request.

How fast is it?  It looks generally good though.

> @@ -0,0 +1,64 @@
+#!/usr/bin/env python3
+
+import sys,getopt,re
+
+def main(argv):
+    inputfile = ''
+    maxLineLength = 80
+    maxFigureLineLength = 65
+    foundError = False
+
+    try:
+        opts,args = getopt.getopt(argv,"i:lf")

Doesn't this need to be `"i:l:f:"`?  I see that you haven't tested these so...

> @@ -0,0 +1,64 @@
+#!/usr/bin/env python3

can you run autopep8 over this?

> +        elif opt == "-f":
+            maxFigureLineLength = arg
+
+    insideFigure = False
+    beforeAbstract = True
+    with open(inputfile,'U') as draft:
+        linecounter = 1
+        lines = draft.readlines()
+        for line in lines:
+            line = line.rstrip('\r\n')
+            linenumber = linecounter
+            linecounter += 1
+
+            ## Skip everything before abstract
+            if beforeAbstract:
+                matchObj = re.match('^--- abstract',line)

you can compile regexes that you run multiple times so that they run more quickly:

```py
abstract = re.compile('^--- abstract')
# ...
matchObj = abstract.match(line)
```

> +            ## Skip everything before abstract
+            if beforeAbstract:
+                matchObj = re.match('^--- abstract',line)
+                if matchObj:
+                    beforeAbstract = False
+                continue
+
+            ## Skip tables
+            matchObj = re.match('^\s*\|',line)
+            if matchObj:
+                continue
+
+            ## Toggle figure state
+            matchObj = re.match('^[~`]{3,}',line)
+            if matchObj:
+                insideFigure = False if insideFigure else True

```py
insideFigure = not insideFigure
```

> +        linecounter = 1
+        lines = draft.readlines()
+        for line in lines:
+            line = line.rstrip('\r\n')
+            linenumber = linecounter
+            linecounter += 1
+
+            ## Skip everything before abstract
+            if beforeAbstract:
+                matchObj = re.match('^--- abstract',line)
+                if matchObj:
+                    beforeAbstract = False
+                continue
+
+            ## Skip tables
+            matchObj = re.match('^\s*\|',line)

Use `'^\s*(?:\||{:)'` here so that you skip captions as well.

> +            if matchObj:
+                insideFigure = False if insideFigure else True
+                continue
+
+            ## Check length
+            length = len(line)
+            limit = maxFigureLineLength if insideFigure else maxLineLength
+            if length > limit:
+                foundError = True
+                print("{0}: Line is {1} characters; limit is {2}".format(linenumber,length,limit))
+                print(line)
+
+    sys.exit( 1 if foundError else 0)
+
+if __name__ == "__main__":
+   main(sys.argv[1:])

No EOL.

> @@ -16,13 +16,5 @@ latest:: lint
 .PHONY: lint
 lint::
 	@err=0; for f in draft-*.md ; do \
-	  if cat "$$f" | (l=0; while read -r a; do l=$$(($$l + 1)); echo -E "$$l:$$a"; done) | \
-	     sed -e '1,/--- abstract/d;/^[0-9]*: *|/d' | tr -d '\r' | grep '^[0-9]*:.\{81\}'; then \
-	    echo "$$f contains a line with >80 characters"; err=1; \
-	  fi; \
-	  if cat "$$f" | (l=0; while read -r a; do l=$$(($$l + 1)); echo -E "$$l:$$a"; done) | \
-	     sed -e '/^[0-9]*:~~~/,/^[0-9]*:~~~/p;/^[0-9]*:```/,/^[0-9]*:```/p;d' | \
-	     tr -d '\r' | grep '^[0-9]*:.\{66\}'; then \
-	    echo "$$f contains a figure with >65 characters"; err=1; \
-	  fi; \
+	  ./.lint.py -i $$f || err=1; \

Rather than iterate here, could you instead iterate in the script?  It would be more natural to drop the `-i` thing and iterate over the remaining arguments that getopt doesn't consume.

> @@ -0,0 +1,64 @@
+#!/usr/bin/env python3
+
+import sys,getopt,re
+
+def main(argv):
+    inputfile = ''
+    maxLineLength = 80
+    maxFigureLineLength = 65
+    foundError = False
+
+    try:
+        opts,args = getopt.getopt(argv,"i:lf")
+    except getopt.GetoptError:
+        print('.lint.py -i <input_file> [-l <line-length>] [-f <figure-line-length>]')

sys.stderr.write('...\n') for errors

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#pullrequestreview-86071151

---

From: Mike Bishop <[email-masked]>
Date: Wed, 03 Jan 2018 23:26:06 +0000 (UTC)

MikeBishop commented on this pull request.

+#!/usr/bin/env python3
+
+import sys,getopt,re
+
+def main(argv):
+    inputfile = ''
+    maxLineLength = 80

No -- ':' indicates that the argument is mandatory, and these have defaults.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#discussion_r159552673

---

From: Mike Bishop <[email-masked]>
Date: Wed, 03 Jan 2018 23:29:50 +0000 (UTC)

+            if beforeAbstract:

Somewhat more opaque, but sure.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#discussion_r159553173

---

From: Martin Thomson <[email-masked]>
Date: Wed, 03 Jan 2018 15:32:15 -0800

+#!/usr/bin/env python3
+
+import sys,getopt,re
+
+def main(argv):
+    inputfile = ''
+    maxLineLength = 80

>From `man getopt`:

```
       -o, --options shortopts
              The short (one-character) options to be recognized.  If this option is not found, the  first  parameter
              of  getopt  that does not start with a '-' (and is not an option argument) is used as the short options
              string.  Each short option character in shortopts may be followed by one colon to  indicate  it  has  a
              required  argument,  and by two colons to indicate it has an optional argument.  The first character of
              shortopts may be '+' or '-' to influence the way options are parsed and output is generated  (see  sec‐
              tion SCANNING MODES for details).
```

Colon indicates whether the option is followed by an argument.  What you have here lets you do `./.lint.py -l`, but not `./.lint.py -l 20` (the "20" becomes an unattributed argument).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#discussion_r159553500

---

From: Mike Bishop <[email-masked]>
Date: Wed, 03 Jan 2018 15:53:48 -0800

@MikeBishop pushed 2 commits.

3d296ef  Most of Martin's feedback
53d9932  Use argparse instead of getopt; multiple files at once

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032/files/f5a845bdb2eda25088dd646e3fed6b179bdb34dc..53d9932ef2a9728f2dc5b2af0b5834b994cc5034

---

From: Mike Bishop <[email-masked]>
Date: Wed, 03 Jan 2018 15:58:25 -0800

@MikeBishop pushed 1 commit.

1b08f18  Stderr here too

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032/files/53d9932ef2a9728f2dc5b2af0b5834b994cc5034..1b08f184e4cee9c7af4350fa7053cc2ade2e0f36

---

From: Martin Thomson <[email-masked]>
Date: Wed, 03 Jan 2018 16:06:57 -0800

OK, this seems good.  A modest improvement on linux.  I'm going to autopep8 this and drop the extra comma.

```
$ git co pylint
$ time make lint

real    0m0.691s
user    0m0.395s
sys     0m0.355s
$ git co master
$ time make lint

real    0m1.008s
user    0m0.588s
sys     0m0.544s
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#issuecomment-355163158

---

From: Martin Thomson <[email-masked]>
Date: Thu, 04 Jan 2018 00:07:35 +0000 (UTC)

@martinthomson pushed 2 commits.

1eea074  autopep8
125b71b  Remove comma

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032/files/1b08f184e4cee9c7af4350fa7053cc2ade2e0f36..125b71b6997495e40da16fecf8a8c842381eb16e

---

From: Mike Bishop <[email-masked]>
Date: Wed, 03 Jan 2018 16:19:19 -0800

Yeah, after this the bulk of the time spent on WSL is in make itself, not the lint phase.

```
mbishop@sea-wpwin:/mnt/c/Users/mbishop/Documents/GitHub/quic$ time make lint

real    0m8.921s
user    0m0.422s
sys     0m7.500s
mbishop@sea-wpwin:/mnt/c/Users/mbishop/Documents/GitHub/quic$ time ./.lint.py draft-ietf-quic-*.md

real    0m0.129s
user    0m0.031s
sys     0m0.063s
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#issuecomment-355165089

---

From: Martin Thomson <[email-masked]>
Date: Wed, 03 Jan 2018 16:19:56 -0800

> +parser.add_argument('-f', dest='maxFigureLineLength', default=65)
+
+args = parser.parse_args()
+
+foundError = False
+
+for inputfile in args.files:
+    insideFigure = False
+    beforeAbstract = True
+    with open(inputfile,'U') as draft:
+        linecounter = 1
+        lines = draft.readlines()
+
+        abstract = re.compile('^--- abstract')
+        table = re.compile('^\s*(?:\||{:)')
+        figure = re.compile('^[~`]{3,}')

You can drop the `,` here.

> @@ -14,5 +14,13 @@ endif
 
 latest:: lint
 .PHONY: lint
+
+PYTHON := $(shell which python3)
+ifeq ($(PYTHON),)
+PYTHON := $(shell which python)
+endif
+
+ifneq ($(PYTHON),)

I don't think that we need this check.  We need python for xml2rfc.

> @@ -14,5 +14,13 @@ endif
 
 latest:: lint
 .PHONY: lint
+
+PYTHON := $(shell which python3)
+ifeq ($(PYTHON),)
+PYTHON := $(shell which python)
+endif

Try:

```make
PYTHON := $(or $(shell which python3),$(shell which python))
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#pullrequestreview-86526456

---

From: Martin Thomson <[email-masked]>
Date: Thu, 04 Jan 2018 00:21:15 +0000 (UTC)

@martinthomson pushed 1 commit.

a629788  autopep8

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032/files/9f84c88b8264c97d0255609b950074e04f41ddee..a6297888e1202ac76a8d102205eeb810df4c1e68

---

From: Mike Bishop <[email-masked]>
Date: Wed, 03 Jan 2018 16:37:08 -0800

7abbc4a  More compact version probing

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032/files/a6297888e1202ac76a8d102205eeb810df4c1e68..7abbc4a609602882e4b5ba3b5b3d7c190f29bbf8

---

From: Mike Bishop <[email-masked]>
Date: Wed, 03 Jan 2018 16:39:14 -0800

e323a81  Martin's hated comma came back

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032/files/7abbc4a609602882e4b5ba3b5b3d7c190f29bbf8..e323a81186fbcd8ba097e0f3300be6c1021c722e

---

From: Martin Thomson <[email-masked]>
Date: Tue, 16 Jan 2018 03:27:05 +0000 (UTC)

Merged manually.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#issuecomment-357844072

---

From: Martin Thomson <[email-masked]>
Date: Mon, 15 Jan 2018 19:27:06 -0800

Closed #1032.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1032#event-1425377378