Skip to content
New issue

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

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

Already on GitHub? # to your account

allow gf (go to file) command also for __() method #21

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thyseus
Copy link

@thyseus thyseus commented Feb 10, 2021

(which is an alias for trans())

@noahfrederick
Copy link
Owner

Thanks! This addition is welcome, but the proposed code change has a couple problems:

  1. It overwrites the original function, so we've lost support for trans() and trans_choice().
  2. The new pattern covers a non-existent ___choice() function in addition to __().

Please extend the existing pattern to match __() instead of creating another function. This is also a good opportunity to include test coverage for this functionality. See existing tests for examples.

Let me know if you need further guidance.

@thyseus
Copy link
Author

thyseus commented Feb 16, 2021

@noahfrederick Yes, i need some guidance. I almost got the regular expression right to also 'listen' on __() - but he is not able to resolve the file properly using my technique. Could you please take a look at b20a49b ? Once this works i want to try to write a test for that myself :)

Copy link
Owner

@noahfrederick noahfrederick left a comment

Choose a reason for hiding this comment

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

Sorry for being slow to get around to this! Please see my comment.

@noahfrederick
Copy link
Owner

Do you want to try adding test coverage? It would involve using vader.vim to make assertions about the output of laravel#goto#filetype_php() and laravel#goto#filetype_blade() when the cursor is on different snippets of PHP code. The tests would look something like:

Before (in a laravel PHP buffer):
  tabedit test/fixtures/laravel-8/app/Example.php
  ...add buffer contents

After (clean up buffer):
  bdelete

Execute (Go to translation file):
  AssertEqual laravel#goto#filetype_php(), 'Elanguage some/path.php'

...

The test suite actually involves running the tests against a new Laravel project. The setup script is here, and you can see how a test like this works here, and you can run the test suite with:

vim -Nu test/fixtures/init.vim -c 'Vader! test/*'

@thyseus
Copy link
Author

thyseus commented Mar 1, 2021

Yes - i will try in this running week. Awesome that there is a testing suite for vim... :)

@thyseus
Copy link
Author

thyseus commented Mar 4, 2021

Hey @noahfrederick

i think i am far to the goal, but it seems like the vim instance that vader.vim starts with your instructions does not have the vim-laravel module loaded. Setting the cursor the first a of auth on {{ __('auth.failed'); }} and hitting "gf" does not do what is expected. How can i ensure that the vim-laravel project has been loaded? Doing this steps "manually" without vader.vim does have the expected behavior.

Thanks a lot !

Copy link
Owner

@noahfrederick noahfrederick left a comment

Choose a reason for hiding this comment

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

it seems like the vim instance that vader.vim starts with your instructions does not have the vim-laravel module loaded.

That's possible. How are you invoking vader.vim? The test/fixtures/init.vim init file assumes all the required plug-ins are installed to the current working directory. You will probably need to adjust that. Or you could just run the tests using your own vimrc instead, i.e., vim -c 'Vader! test/*'. That's probably easier.

@@ -462,3 +465,27 @@ Execute (edit routes):

Expect (generic template):
<?php

Copy link
Owner

Choose a reason for hiding this comment

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

Let's move these new tests to their own test file, tests/goto.vader (matching the autoload script name), since we're testing a different type of functionality.

After (clean up buffer):
bdelete

Before (blade file with trans() function):
Copy link
Owner

Choose a reason for hiding this comment

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

My understanding is that Before and After blocks are executed again for every test in the test file, so there should only be one of each per file. You can do additional setup for a single test in an Execute block instead.

Comment on lines +472 to +476
Do (Move to (a)uth.failed, jump to translation file):
i{{ __('auth.failed') }}\<Esc>0fagf

Execute:
AssertEqual expand('%'), 'resources/lang/auth.php'
Copy link
Owner

Choose a reason for hiding this comment

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

It's good to exercise the gf mapping at least once, as you have here, but it would also be valuable to test the output of laravel#goto#filetype_php() and laravel#goto#filetype_blade() directly. They output strings that are executed as navigation commands:

  AssertEqual laravel#goto#filetype_blade(), 'Elanguage en/auth'

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants