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 #22 - Better support for vendor based .php files. #31

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

Conversation

rabrowne85
Copy link

Fixing issue #22:

  1. Determine if we're in the vendor folder.
  2. Use regex to search for the format /vendor/{package}/{locale}/filename.php
  3. Use the locale as the new "directory" name (N.B. renamed bundle[directory] to bundle[locale])
  4. Remove the locale from the filename.

This will convert the following: vendor/some-package/en-GB/filename.php to this:

{
    en-GB : {
        some-package/filename: {
            ...
       }
   }
}

I don't use the .json option, so not sure if this has the same problem with the vendor packages.

I haven't written a new test case for this, but can do if it's required.

I'm not sure if this is classified as a breaking change, as it fixes something that wouldn't have been possible to access previously.

Any questions, just let me know.

Determine if we're in the vendor folder and if so use the regex to determine the "locale" and then strip this out of the filename.
@luisdalmolin
Copy link
Member

@rabrowne85 Sorry it took me this long to check your PR. If you could write a test case for this it would be good.

@luisdalmolin
Copy link
Member

This is a great addition, btw!

@rabrowne85
Copy link
Author

@luisdalmolin Thanks 👍 I've added some dummy vendor package language files to the test suite and then extended the existing tests to ensure that they now pass.

The only one I've struggled with is the dependency test at the end of the suite. For this I've duplicated the namespace folder without the vendor files and left the test alone.

Let me know if there's anything else you want adding/changing.

@luisdalmolin
Copy link
Member

@rabrowne85 The new assertions are looking good, but the builds are failing, though. From a quick look it seems like a few safety checks before running some stuff could be enough.

You can test it locally by running npm run test.

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

Successfully merging this pull request may close these issues.

2 participants