Pipeto --as_file #1476
Pipeto --as_file #1476
Conversation
ryneeverett
force-pushed
the
pipeto-file
branch
2 times, most recently
from
7d11c9d
to
9203dbb
Compare
|
None of these issues is actually changed from the way it is in master, I've just made modifications to those lines which has triggered codeclimate. Do you really want me to "correct" these issues in this PR? I also don't think codeclimate is being used correctly in this project. These seem to be more "red flags" that are worth noticing than bad practices that should be blockers. Aside from the masking of the built-in format which I agree is not best practice (but not worth changing at this point) the other two are just worth noticing:
|
|
I agree. Note that I didn't actually review this myself but its an artefact of me fixing the automation on github. I don't expect you to fix any of these. |
|
please rebase to keep this one alive. cheers! |
|
Something I did notice with the current approach is that asynchronous commands will not generally work because the temporary file will be removed before it is loaded. One example is that when piping into a tab of an existing browser process (for example firefox), the browser invocation does not block. This use case can be solved by piping into a wrapper script that sleeps for a second until the tab is loaded. The file will still be removed but it shouldn't matter. Another example is that usage of pipeto's Now that I've typed this I may as well press enter, but I think I'm going to look into adding a commit which changes the behavior to wait a few seconds before removing the temporary file, regardless of when the process exits. |
| ui.notify(self.done_msg) | ||
| await ui.apply_command(ExternalCommand(cmd, | ||
| stdin=mail, | ||
| shell=self.shell, |
There was a problem hiding this comment.
| @@ -648,7 +655,7 @@ class PipeCommand(Command): | |||
| repeatable = True | |||
|
|
|||
| def __init__(self, cmd, all=False, separately=False, background=False, | |||
| shell=False, notify_stdout=False, format='raw', | |||
| shell=False, notify_stdout=False, format='raw', as_file=False, | |||
There was a problem hiding this comment.
| @@ -978,7 +999,7 @@ async def apply(self, ui): | |||
| tempfile_name = tmpfile.name | |||
| self.attachment.write(tmpfile) | |||
|
|
|||
| def afterwards(): | |||
| def afterwards(*args): | |||
There was a problem hiding this comment.
The most notable use case is piping html to a browser without extra scripts such as those shared in pazz#789.
While `pipeto --format mimepart` pipes whichever part is currently selected, the 'html' and 'plain' formats override the selected mime part (and preferences in settings). This is useful because the mime type you want displayed in alot isn't necessarily the one you want piped and also for setting keybindings to pipe specific mime types to specific applications (plain -> text editor, html -> web browser).
ryneeverett
force-pushed
the
pipeto-file
branch
from
5fe5b29
to
bf85891
Compare
When the mimetree is toggled on and a mime part is focused, pipeto should pipe the focused mime part rather than the currently selected part. This is only applicable to --format's that pipe a single part, which are decoded and mimepart.
If an exception occurs ret is already defined and proc is not.
There just doesn't seem to be any way to invoke a usable editor with a subprocess that's passed stdin. Therefore this option doesn't literally pipe mail into the command but writes mail to a temporary file and passes the filename at the end of the external command.
Many browsers (including chromium) require that files end in the '.html' extension in order to render them. See <https://bugs.chromium.org/p/chromium/issues/detail?id=777737>. When using `pipeto --format=mimepart --as_file`, on an html mime part, let's automatically set the file extension. This eliminates the need for additional user scripts referenced in issues such as pazz#789 and pazz#1153.
This is an asynchronous method so there's little harm in sleeping. This fixes two different use cases in which the file could otherwise be removed before it is opened: - When piping into a tab of an existing browser process (for example firefox), the command does not block. - When using pipeto's --background flag, the command will be invoked asynchronously.
This message could now display for several different external commands besides editor.
Prior to this branch, `shell=True` was passed to Popen regardless of whether `--shell` was passed. I'm not sure why this was the case but it wasn't a problem. Now that we're passing to ExternalCommand, this doesn't work because ExternalCommand does it's own manipulation of cmd when shell=False which is incompatible with a list-style cmd.
ryneeverett
force-pushed
the
pipeto-file
branch
from
bf85891
to
aa04a72
Compare
This branch is based on
#1475, #1480, and#1481 so I wouldn't recommend a thorough review yet.Rather than literally piping mail into a command, this option writes to a temporary file which is passed to the command. For some applications this is the only feasible option.
Only the tiny final commit adding browser support actually depends on mime support but I don't think the overhead of this feature can be justified without the use case of piping to a browser.