Move to pandoc for markdown and premailer for CSS #16
Conversation
…iple); using premailer for CSS inlining.
There was a problem hiding this comment.
I'm okay using premailer, since it does indeed appear to be better-maintained than pynliner.
I'm less confident about making pandoc a requirement; I would generally not use pypandoc's statically-compiled gigantic pandoc binary, and I imagine a lot of people don't want the entire GHC toolchain in order to send email. Perhaps making it an optional dependency and using it if it's present, and otherwise using the pure-python Markdown interpreter would be a better option. I'm also pretty sure I just found a security bug in pypandoc in 30 seconds of inspection, which is sad-making.
See below for some style notes and bugs.
| if not text.startswith('!m'): | ||
| return None | ||
| text = re.sub('\s*!m\s*', '', text, re.M) | ||
| f = tempfile.NamedTemporaryFile(suffix='_panmail.css') |
There was a problem hiding this comment.
temporary files should always be used as context managers so that they get cleaned up on pypy
There was a problem hiding this comment.
This bothered me as well -- to be sure. I did initially implement it that way. The reason I changed was to try and change less of the underlying code structure. I think handling the signature separately is where I ran into an awkward situation so found my way around it by just manually closing the file later (which also deletes it). Can do better on that though.
| md += '<br />'.join(signature.split('\n')) | ||
| md += '</p></div>' | ||
| message = pypandoc.convert_text(pre_signature, 'html5', format='md', \ | ||
| extra_args=["--css="+f.name, "--self-contained", "--metadata=pagetitle:'email'"]) |
There was a problem hiding this comment.
preferred style would be something like
message = pypandoc.convert_text(
pre_signature, 'html5', format='md',
extra_args=['--css=' + f.name, ...]
)
| return None | ||
| text = re.sub('\s*!m\s*', '', text, re.M) | ||
| f = tempfile.NamedTemporaryFile(suffix='_panmail.css') | ||
| if config.css: f.write(config.css) |
There was a problem hiding this comment.
never do one-line if statements in python
| md = pynliner.fromString(md) | ||
| message = MIMEText(md, 'html', _charset="UTF-8") | ||
| message = pypandoc.convert_text(text, 'html5', format='md', \ | ||
| extra_args=["--css="+f.name, "--self-contained", "--metadata=pagetitle:'email'"]) |
There was a problem hiding this comment.
see above comment regarding correct indentation for multiline function call arguments
| extra_args=["--css="+f.name, "--self-contained", "--metadata=pagetitle:'email'"]) | ||
| message = premailer.transform(message) # In-line the CSS. | ||
| message = MIMEText(message, 'html', _charset="UTF-8") | ||
| f.close() |
There was a problem hiding this comment.
this will no longer be necessary when you switch to a context manager
| print(rebuilt.as_string()) | ||
| elif args.sendmail_passthru: | ||
| cmd = c.sendmail.split() + ['-f', args.envelope_from] + args.addresses | ||
| cmd = c.sendmail.split() + ['-G', '-i', '-f', args.envelope_from] + args.addresses |
There was a problem hiding this comment.
-G would need to be controlled by a config flag since it would only be appropriate when using this as a postfix tool
|
|
||
| proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, shell=False) | ||
| proc.communicate(rebuilt.as_string()) | ||
| proc.communicate(rebuilt.as_string().encode()) |
There was a problem hiding this comment.
calling encode without an argument has pretty confusing behavior on a lot of systems since it tries to guess the default encoding
There was a problem hiding this comment.
Arguable, I think the default is actually the least confusing possible behavior. I would opt instead for just dropping support of legacy python, which is really the root cause of confusion around strings/bytes. Especially with the additional decode bound method that converts the html from base64 to usual text encoding.
|
Regarding the size of Could you elaborate on the suspected security vulnerability in Also, LOL: "All checks have failed." I bet Travis had fun trying to build |
|
Just one note, moving out from pynliner would help muttdown from the contrib to main area of the Debian archive. premailer is not yet in Debian, I read its license and it looks good (pending approval from ftp master) to inclusion in main. I did not check premailer dependencies (if any). If muttdown switches i'd package premailer, and deps, and when all deps are in Debian i'd switch muttdown from contrib to main. |
3 participants
This is a proof of principle, but fully functional pull request. For my purposes, using the pandoc variant of markdown just makes the most sense. I also think it is the most robust and featureful so require it as an option in any markdown based tool I use. It was pretty easy to implement that in muttdown, leveraging other packages already available.
pylineralso does not seem all that well maintained so I opted forpremailerto perform the CSS in-lining instead, which seems reasonable since it is well tested and designed for this very purpose.I currently have this version of muttdown set up as a filter on my local Postfix install and it works great. The last detail at this stage I would like to see changed is to remove the
!msigil from thetext/plainpart. That is not currently written back into the multipart object.So, I am going to continue down this path regardless of whether or not you would like to merge, but if this all seems in line with what you wanted to do then I am happy to have that happen in muttdown's birth repo (this one).