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

allow model_locations to have glob patterns #1059

Merged
merged 4 commits into from
Sep 20, 2020
Merged

allow model_locations to have glob patterns #1059

merged 4 commits into from
Sep 20, 2020

Conversation

isaackearl
Copy link
Contributor

This if statement check for a valid directory was happening to early. It was checking the string that contained the wildcards to see if it is a valid directory and always failing. Need to check after the foreach starts. fixes #1058 .

Summary

Glob patterns currently don't work. The if statement that checks if the path is a directory happens before the glob helper call.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

This if statement check for a valid directory was happening to early.  It was checking the string that contained the wildcards to see if it is a valid directory and always failing.  Need to check after the foreach starts. fixes #1058 .
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!


Would you be up for a test for this?

@isaackearl
Copy link
Contributor Author

Thanks @mfn .. I think I could be up for a test tomorrow or the next day. To be honest I've been on a rampage tonight trying to get some things working and it's now 5 AM. :)

@mfn
Copy link
Collaborator

mfn commented Sep 17, 2020

it's now 5 AM. :)

💥

But yes, I've heard of that. Humans need 💤 and such 😏

@mfn
Copy link
Collaborator

mfn commented Sep 17, 2020

Github Actions seems to hang, but nothing on https://www.githubstatus.com/

@isaackearl
Copy link
Contributor Author

@mfn is there a way to force the checks to re-run? Seems strange that they hung.

@isaackearl
Copy link
Contributor Author

@mfn I wrote a test, but may need some feedback about where I put it etc. I tried to follow the pattern of the other tests.

@isaackearl
Copy link
Contributor Author

@mfn So I'll probably need some help finishing this test. It works for me locally when I run using various php versions, however when I run it included with all the rest of the tests, it fails asserting that 'Written new phpDocBlock to' is there. In other words: run the test by itself 👍 ... run the test with all the rest of them 👎 .

@mfn
Copy link
Collaborator

mfn commented Sep 18, 2020

Sure, I'll take a look over the weekend!

I'll try and see if I can re-start the tests by closing/opening the PR.

@mfn mfn closed this Sep 18, 2020
@mfn mfn reopened this Sep 18, 2020
@mfn
Copy link
Collaborator

mfn commented Sep 19, 2020

. It works for me locally when I run using various php versions, however when I run it included with all the rest of the tests

I don't know how it even worked for you, as the test neither in isolation nor together with the whole suite worked for me, simply because the "current working directory" as not the one expected.

I pushed a fix (and updated the changelog), all green now.

Are you using windows? Locally I use OSX and the GHA is using Linux, and they behaved the same.

@isaackearl
Copy link
Contributor Author

@mfn
I'm on Mac os x as well. Now that I'm seeing your changes I have no idea how this is STILL working for me locally when I run the test individually. I am running my tests through php storm.
Screen Shot 2020-09-19 at 3 28 26 PM

I'm on php 7.4.9 installed via homebrew.

Regardless, I appreciate you updating it and making it truly work.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

ok, great and thanks; also using phpstorm here 😏

Just wanted to make sure there's no bug hidden, but PR LGTM too.

@barryvdh IMHO this is good to merge!

@barryvdh barryvdh merged commit 1044f46 into barryvdh:master Sep 20, 2020
@isaackearl isaackearl deleted the patch-1 branch September 21, 2020 03:11
@NickSdot
Copy link

Tested. Works here as expected. Thanks!

@mfn
Copy link
Collaborator

mfn commented Oct 20, 2020

@NickSdot thanks for the feedback 👍

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.

Config says glob patterns are supported, but they don't seem to be
4 participants