Exit jrnl if no text entered into editor

[alichtman]

Fix #589

Pretty self-explanatory. I was trying to think of a creative way to test this but I don't think we can automate exiting the editor cleanly (and judging by the lack of tests for anything relating to an editor, I think everyone else has come to the same conclusion that I did.)

Checklist

• The code change is tested and works locally.
• Tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?

[pspeter]

I see two ways to test this (and other related functionality):

1. (easy) Mock the whole util.get_text_from_editor() function to return a predefined string ("" in this case)
2. (bit harder) Mock the subprocess.call() call to put that predefined string into the tmpfile.

[alichtman]

Mock the whole util.get_text_from_editor() function to return a predefined string ("" in this case)

This solution makes sense to me. I'm thinking this is somewhat along the lines of how I'll implement it, but I'm a little shaky on the details. (The following is left as notes for myself when I have time to finish thinking about this.)

The feature test may look like this:

 Scenario: Writing an empty entry from the editor
        Given we use the config "basic.yaml"
        When we open the editor and exit
        Then there should be no output

Where open the editor and exit will correspond to a @when decorator in steps/core.py that does something after a line with unittest.patch('util.get_text_from_editor', side_effect=return_empty_string).

Note that this would mean the test will require no output, when in reality, the output would be [Nothing saved to file].

Note: I'm not sure that the test I've written actually does anything at the moment...

[pspeter]

I would be a bigger fan of patching the concrete subprocess call. Generally you should avoid patching your own code, especially if it has side-effects (like the printed output). I mentioned it earlier because it was easier but I didn't consider it would change the output.

[alichtman]

Yep, agreed. Will fix when I've got some time.

[alichtman]

It's unclear to me why this test outputs the following on stderr:

On this branch, when I open the editor, write nothing and exit, I get this:

Edit: Haha, so it passes on Travis? I hate computers sometimes.

[pspeter]

There we go :D

[pspeter]

It's not super necessary I guess but after moving the sys.exit() down, you could also add a second test that is exactly the same as this one but uses the basic.yaml and When we run jrnl and enter "" (and add @when('we run "{command}" and enter ""') to the corresponding step function...) to also test the early exit when no editor is used.

But otherwise it looks good now 👍

[alichtman]

I honestly don't think that's necessary. Who's going to be running jrnl ""?

[pspeter]

"" denotes an empty string. It's the same scenario as above when somebody closes the editor without writing anything, only without the editor. But without an editor, this line will be called:

jrnl/jrnl/cli.py

Line 202 in 4318da5

raw = sys.stdin.read()

But this can also be exited without writing anything, leaving you with an empty string again. They would still call jrnl without quotes.

[alichtman]

Oh, so this would happen if someone tried to cat an empty file into jrnl on stdin?

[pspeter]

That too, I assume. But even if you don't pipe anything into jrnl, it will start an input stream at that line and let you type something in your terminal. You can input multiple lines and then stop the stream with ctrl+d.

[alichtman]

This test breaks for some unknown reason. Will take a look at it again when I have time. I have to do some other work...

[pspeter]

Try this

[alichtman]

@wren: This PR is also ready for review.

[wren]

💯

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.