Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29984580: Issue 4413 - Merge formmail.py and formmail2.py (Closed)

Created:
Jan. 17, 2019, 8:42 a.m. by rhowell
Modified:
Jan. 26, 2019, 3:27 a.m.
Reviewers:
Vasily Kuznetsov
Base URL:
https://hg.adblockplus.org/sitescripts/
Visibility:
Public.

Description

Issue 4413 - Merge formmail.py and formmail2.py

Patch Set 1 #

Patch Set 2 : Start the README #

Total comments: 12

Patch Set 3 : Continue writing README #

Total comments: 3

Patch Set 4 : More work on README #

Total comments: 4

Patch Set 5 : Nits & formatting on README #

Unified diffs Side-by-side diffs Delta from patch set Stats (+367 lines, -461 lines) Patch
M .hgignore View 1 1 chunk +2 lines, -1 line 0 comments Download
M .sitescripts.example View 2 chunks +0 lines, -4 lines 0 comments Download
A sitescripts/formmail/README.md View 1 2 3 4 1 chunk +68 lines, -0 lines 0 comments Download
R sitescripts/formmail/template/eyeo.mail View 1 1 chunk +0 lines, -10 lines 0 comments Download
M sitescripts/formmail/test/test_formmail.py View 2 chunks +149 lines, -43 lines 0 comments Download
R sitescripts/formmail/test/test_formmail2.py View 1 chunk +0 lines, -199 lines 0 comments Download
M sitescripts/formmail/web/formmail.py View 1 chunk +146 lines, -30 lines 0 comments Download
R sitescripts/formmail/web/formmail2.py View 1 chunk +0 lines, -171 lines 0 comments Download
M tox.ini View 1 chunk +2 lines, -3 lines 0 comments Download

Messages

Total messages: 10
rhowell
Jan. 17, 2019, 8:42 a.m. (2019-01-17 08:42:12 UTC) #1
Vasily Kuznetsov
Hi Rosie, LGTM. However, note that the path of the handler is now different and ...
Jan. 17, 2019, 11:28 a.m. (2019-01-17 11:28:13 UTC) #2
rhowell
On 2019/01/17 11:28:13, Vasily Kuznetsov wrote: > Please coordinate with Ops regarding this or let ...
Jan. 18, 2019, 12:46 a.m. (2019-01-18 00:46:07 UTC) #3
Vasily Kuznetsov
Hi Rosie! Great that you're coordinating with Paco and good start on the README. I ...
Jan. 18, 2019, 4:52 p.m. (2019-01-18 16:52:13 UTC) #4
rhowell
Hi Vasily! Thanks for all the great feedback, it's much appreciated. I just have a ...
Jan. 18, 2019, 8:08 p.m. (2019-01-18 20:08:08 UTC) #5
Vasily Kuznetsov
Hi Rosie! All going in the right direction so far. See my replies below. Cheers, ...
Jan. 21, 2019, 1:54 p.m. (2019-01-21 13:54:18 UTC) #6
rhowell
Hey Vasily! Thanks for your help. Hopefully it's looking better now? https://codereview.adblockplus.org/29984580/diff/29984612/sitescripts/formmail/README.md File sitescripts/formmail/README.md (right): ...
Jan. 22, 2019, 10:53 p.m. (2019-01-22 22:53:54 UTC) #7
Vasily Kuznetsov
Hi Rosie! Great. I think we have all the information we need in the README ...
Jan. 23, 2019, 12:23 p.m. (2019-01-23 12:23:13 UTC) #8
rhowell
Hey Vasily, Good idea renaming those imaginary files. The new names are much more useful, ...
Jan. 23, 2019, 9:40 p.m. (2019-01-23 21:40:26 UTC) #9
Vasily Kuznetsov
Jan. 24, 2019, 1:16 p.m. (2019-01-24 13:16:28 UTC) #10
Hi Rosie!

LGTM!

On 2019/01/23 21:40:26, rhowell wrote:
> 
> I edited all the lines down to 79 characters, but just wanted to confirm that
> this is a hard rule, no exceptions? I usually check how a README is rendered
on
> https://dillinger.io/, and some of the lines are very uneven now. Especially
> links, which (AFAIK) cannot be separated by a line break. Also, many of our
> other READMEs go over by a few characters here and there. Just wanted to
> double-check.

It's not a completely hard rule, exceptions can me made, especially for URLs and
pre-formatted text. The current version looks good though: I checked it using
Grip (https://pypi.org/project/grip/) that essentially uses GitHub renderer and
the output looks just like it should.

Cheers,
Vasily

Powered by Google App Engine
This is Rietveld