Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix editor config when an argument with a space is used #953

Merged
merged 5 commits into from
May 23, 2020

Conversation

wren
Copy link
Member

@wren wren commented May 17, 2020

Fixes #581

When the 'editor' key in the config contains an argument with a space, it used
to be parsed incorrectly. The example given in the linked issue is vim -f -c 'setf markdown', which would have 'setf and markdown' parsed as two
separate arguments. There was a partial fix implemented, but it left out mac
users due to a faulty platform check.

This fixes those problems and adds a test to confirm.

Checklist

  • The code change is tested and works locally.
  • Tests pass. Your PR cannot be merged unless tests pass. --
    poetry run behave
  • The code passes linting via
    black (consistent code styling). --
    poetry run black --check . --verbose --diff
  • The code passes linting via pyflakes
    (logically errors and unused imports). -- poetry run pyflakes jrnl features
  • 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?

@wren wren force-pushed the editor-config-581 branch from 476473a to 455720f Compare May 17, 2020 01:46
@wren
Copy link
Member Author

wren commented May 17, 2020

I forgot that our editor mock hangs on Windows. Trying to decide if we should fix or skip.

@micahellison can you look to see if it's a quick fix on Windows?

@micahellison
Copy link
Member

I forgot that our editor mock hangs on Windows. Trying to decide if we should fix or skip.

@micahellison can you look to see if it's a quick fix on Windows?

I'm checking it out but I'm stumped. Travis hangs on When we open the editor and enter "lorem ipsum" but my local Windows 10 machine doesn't. However, weirder things are happening here.

When I run poetry run behave that step output is:

  Scenario: Sending an argument with spaces to the editor should work  # features/core.feature:42
    Given we use the config "editor-args.yaml"                         # features/steps/core.py:72
    When we open the editor and enter "lorem ipsum"                    # features/steps/core.py:82
    Then the editor should have been called with 5 arguments           # features/steps/core.py:99
    And the editor arguments should contain "vim"                      # features/steps/core.py:104
    And the editor arguments should contain "-f"                       # features/steps/core.py:104
    And the editor arguments should contain "-c"                       # features/steps/core.py:104
    And the editor arguments should contain "setf markdown"            # features/steps/core.py:104
      Traceback (most recent call last):
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\model.py", line 1329, in run
          match.run(runner.context)
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features\steps\core.py", line 106, in contains_editor_arg
          assert arg in context.editor_command
      AssertionError

      Captured stderr:
      [Configuration updated to newest version at C:\dev\jrnl\features\configs\editor-args.yaml]
      [Entry added to default journal]

OK, it fails for some reason I don't understand. But when I run poetry run behave features\core.feature the output is different.

  Scenario: Sending an argument with spaces to the editor should work  # features/core.feature:42
    Given we use the config "editor-args.yaml"                         # features/steps/core.py:72
    When we open the editor and enter "lorem ipsum"                    # features/steps/core.py:82
    Then the editor should have been called with 5 arguments           # features/steps/core.py:98
      Traceback (most recent call last):
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\model.py", line 1329, in run
          match.run(runner.context)
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features\steps\core.py", line 100, in count_editor_args
          assert len(context.editor_command) == int(num)
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\runner.py", line 321, in __getattr__
          raise AttributeError(msg)
      AttributeError: 'Context' object has no attribute 'editor_command'

      Captured stderr:
      [Configuration updated to newest version at C:\dev\jrnl\features\configs\editor-args.yaml]
      [Entry added to default journal]

    And the editor arguments should contain "vim"                      # None
    And the editor arguments should contain "-f"                       # None
    And the editor arguments should contain "-c"                       # None
    And the editor arguments should contain "setf markdown"            # None

It sure looks like behave arguments are getting passed to jrnl in this test. For instance, behave -v (showing verbose output) is definitely leading to the test suite somehow running jrnl -v (show version number) as you can see jrnl version 2.4.2 in the "Captured stdout" below:

  Scenario: Sending an argument with spaces to the editor should work  # features/core.feature:42
    Given we use the config "editor-args.yaml"                         # features/steps/core.py:72
    When we open the editor and enter "lorem ipsum"                    # features/steps/core.py:82
    Then the editor should have been called with 5 arguments           # features/steps/core.py:98
      Traceback (most recent call last):
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\model.py", line 1329, in run
          match.run(runner.context)
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features\steps\core.py", line 100, in count_editor_args
          assert len(context.editor_command) == int(num)
        File "c:\users\micah\appdata\local\pypoetry\cache\virtualenvs\jrnl-g_ywnb0w-py3.7\lib\site-packages\behave\runner.py", line 321, in __getattr__
          raise AttributeError(msg)
      AttributeError: 'Context' object has no attribute 'editor_command'

      Captured stdout:
      jrnl version v2.4.2

    And the editor arguments should contain "vim"                      # None
    And the editor arguments should contain "-f"                       # None
    And the editor arguments should contain "-c"                       # None
    And the editor arguments should contain "setf markdown"            # None

I know that seems tangential, but maybe it's a clue? Not sure what to try from here but I'm open to any ideas. If there aren't any, maybe we should skip this on Windows and spin off a new issue to get this fixed down the road.

@wren
Copy link
Member Author

wren commented May 22, 2020

Oh, wow, that looks like a whole big can of worms. 🐛 Maybe it's an issue with behave? Maybe it's our setup?

Either way, I'll just skip this on Windows for now. Thanks for looking into it!

@wren wren force-pushed the editor-config-581 branch 3 times, most recently from fb9fe89 to 551d751 Compare May 23, 2020 22:04
@wren wren force-pushed the editor-config-581 branch from 551d751 to 75cbc71 Compare May 23, 2020 22:13
Copy link
Member

@micahellison micahellison left a comment

Choose a reason for hiding this comment

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

I'm sad we can't get these two tests running in Windows Travis but I removed @skip_win locally and confirmed that they are successfully running on Windows 10. Otherwise, everything looks great.

@micahellison micahellison added the bug Something isn't working label May 23, 2020
@micahellison micahellison merged commit c456b85 into jrnl-org:develop May 23, 2020
@wren wren deleted the editor-config-581 branch July 11, 2020 17:12
wren added a commit that referenced this pull request Jul 25, 2020
* Fix editor config when an argument with a space is used
* skip broken test on windows
* fix jrnl not behaving nicely with testing suite
* fix argument parsing for test suite
* fix one windows test, disable one windows test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"editor" command not working properly with spaces within an argument
3 participants