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

add ability to find models with tags of any type #157

Merged
merged 4 commits into from Dec 21, 2018

Conversation

mokhosh
Copy link
Contributor

@mokhosh mokhosh commented Dec 17, 2018

This adds two new scopes:

scopeWithAllTagsOfAnyType
scopeWithAnyTagsOfAnyType

and to be clean about it, I added one method to HasTags trait and another to the Tag model to convert given arguments to tags of any type and find tags of any type from strings:

HasTags::convertToTagsOfAnyType
Tag::findFromStringOfAnyType

I also added .idea folder to gitignore :)

@freekmurze
Copy link
Member

Thank you for this. 👍 Could you add tests to make sure that this works?

I've remove the .idea again, because this belongs in a global .gitignore.

Copy link
Contributor Author

@mokhosh mokhosh left a comment

Choose a reason for hiding this comment

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

Fair enough :))

@mokhosh
Copy link
Contributor Author

mokhosh commented Dec 18, 2018

I added the tests and they seem fine to me.

But I could not run them, because:

  • it says unknown database laravel-tags although I have created this database and I have created a .env file with mysql credentials.
  • I don't know how to run migrations without artisan.
  • I also had to use php vendor\phpunit\phpunit\phpunit tests instead of just phpunit tests which doesn't seem right
    (don't start throwing tomatos but i don't do tests, look at the bright side, this is how much i love you :D)

Also, should I close this pull request and create a new one with tests or is there a way to add tests to this one?
(it's my first time doing a pull request, now you can start throwing tomatos :D)

@freekmurze
Copy link
Member

freekmurze commented Dec 20, 2018

Travis reports that the t_provides_as_scope_to_get_all_models_that_have_any_of_the_given_tags_with_type is not running properly. https://travis-ci.org/spatie/laravel-tags/jobs/469729909

Could you take a look at that?

it says unknown database laravel-tags although I have created this database and I have created a .env file with mysql credentials.
That's something specific to your system, can't help you with that I'm afraid.

I don't know how to run migrations without artisan.

Running the tests will run the migrations

I also had to use php vendor\phpunit\phpunit\phpunit tests instead of just phpunit tests which doesn't seem right

That's normal, I'll update the readme

don't start throwing tomatos

Never, you're safe here 😄

it's my first time doing a pull request

👍

@mokhosh
Copy link
Contributor Author

mokhosh commented Dec 21, 2018

Travis reports that the t_provides_as_scope_to_get_all_models_that_have_any_of_the_given_tags_with_type is not running properly. https://travis-ci.org/spatie/laravel-tags/jobs/469729909

It was one of the original tests that stopped working because I added model6. Fixed it.

That's something specific to your system, can't help you with that I'm afraid.

It was a very sophisticated problem related to using a - instead of a _ for db name 😄

Thanks for being welcoming. I guess this is ready.

@freekmurze freekmurze merged commit 631a660 into spatie:master Dec 21, 2018
@freekmurze
Copy link
Member

Thank you very much for your work on this! 👍

@freekmurze
Copy link
Member

@mokhosh are you up for sending a PR to the docs for these two new scopes?

@mokhosh
Copy link
Contributor Author

mokhosh commented Dec 21, 2018

Thank you very much for your work on this! 👍

Well, thank you for all your great packages! 👍

@mokhosh are you up for sending a PR to the docs for these two new scopes?

Sure, it's been fun. Also, there's a Farsi proverb that says "Do you know who did the job? The one who made it complete." 😄

@mokhosh
Copy link
Contributor Author

mokhosh commented Dec 21, 2018

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.

None yet

2 participants