Skip to content

Introduce system editor function in \WP_CLI\Utils#302

Merged
scribu merged 6 commits into
wp-cli:masterfrom
goldenapples:system-editor
Feb 14, 2013
Merged

Introduce system editor function in \WP_CLI\Utils#302
scribu merged 6 commits into
wp-cli:masterfrom
goldenapples:system-editor

Conversation

@goldenapples
Copy link
Copy Markdown
Contributor

This is a first pass at trying to address the rough outline of #287 - markdown support and more will have to come separately.

In my initial tests, this seems pretty stable across platforms as far as successfully creating the temp files, preserving the correct line endings and so on, but I have a lot more testing to do.

Does the rough approach look right, or do you see ways to make it more universal?

Adds a function called launch_editor_for_input() which launches the
system's editor to allow file editing of post content, etc. Should
account for system line-endings properly, but probably needs testing on
Windows to be sure. Function returns false if no change (ie user :q!'s
out of vim), or updated content if any change is made.

Also adds two new subcommands to wp post:

  • wp post create --edit Launches the editor to build the post
    content before creating post.
  • wp post edit <id> Edits post 's content in system editor.

Adds a function called `launch_editor_for_input()` which launches the
system's editor to allow file editing of post content, etc. Should
account for system line-endings properly, but probably needs testing on
Windows to be sure. Function returns false if no change (ie user :q!'s
out of vim), or updated content if any change is made.

Also adds two new subcommands to `wp post`:
- `wp post create --edit`	Launches the editor to build the post
							content before creating post.
- `wp post edit <id>`		Edits post <id>'s content in system editor.
Comment thread php/commands/post.php
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'post_content' should not be an associative arg. At most, wp post create should accept post content from STDIN.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wp post create can accept --post_content as an associative argument. It doesn't really make sense for most cases to pass in a string and then edit it, but it seemed wrong to just discard post content is it were passed as an associative arg. Should we throw a warning of some kind if user tries to use both --post_content and --edit arguments?

Accepting content from STDIN is an interesting idea, but probably well outside the scope of this PR. It would probably go along with accepting a filename to read content from. What kind of syntax would you use for that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wp post create can accept --post_content as an associative argument.

Oh, you're right. I forgot about that, since it wasn't mentioned explicitly in the synopsis. Nevermind.

Should we throw a warning of some kind if user tries to use both --post_content and --edit arguments?

Nah; it's weird, but not detrimental.

Also use wp_tempnam to create file, and give the created file a
meaningful name rather than just using system or php's tempfile name.
`fread()` will throw an error if trying to read an empty file (ie if
user deleted all post content or quit the editor on `wp post create`.
Detect that and fail gracefully.
Comment thread php/utils.php Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file_put_contents()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I was getting line ending bugs, which is why I thought the "t" flag in fopen() might be necessary, and there wasn't any way that I could see to pass that through file_put_contents().

I don't think "t" is actually doing anything here though, the bug I was getting was unrelated; caused by trying to trim content and messing up the trailing whitespace. I'll use file_put_contents()/file_get_contents().

Much simpler this way. Also does away with the need to check for empty
output, although now we have to check the return value of
`launch_editor_for_input` strictly against false, since its possible for
it to return an empty string now (editing a post and deleting all the
content).
Comment thread php/utils.php Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using die() directly will send the error to STDOUT, instead of to STDERR and will exit with status 0, rather than status 1.

Better to use WP_CLI::error().

@scribu
Copy link
Copy Markdown
Member

scribu commented Feb 14, 2013

Beyond those minor things, it works pretty well.

Use `WP_CLI::error` for any fatal errors. Output a warning rather than
an error if user aborts the editor (At this point it exits without
performing any action just like an error would, but it shouldn't be
considered an error. Also, its possible that commands using the editor
function will go on to perform other actions.)

Also, whitespace cleanup and minor refactoring... making a helper
method `_edit()` in post.php to make editor functions more reusable.
Use spaces for inner-line spacing
@goldenapples
Copy link
Copy Markdown
Contributor Author

Thanks for all the feedback and help.

I've been trying to think if there's other use cases that this should be expanded for, but I'm not quite sure what capabilities all editors can handle. It might be nice to be able to pass in multiple items to wp post edit, and have all files open in separate tabs/windows in the editor... but can all system editors handle this syntax?

@scribu
Copy link
Copy Markdown
Member

scribu commented Feb 14, 2013

I think being able to edit a single file is enough, especially for the first iteration.

scribu pushed a commit that referenced this pull request Feb 14, 2013
Introduce system editor function in \WP_CLI\Utils
@scribu scribu merged commit 8276184 into wp-cli:master Feb 14, 2013
scribu added a commit that referenced this pull request Feb 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants