Macros are not discoverable / could conflict
Closed this issue · 5 comments
The macros that you have added to the Str class here are not discoverable unless you read the documentation, have prior knowledge of this package, or read the code directly.
They also might conflict with other macros that someone else has already added to their project, possibly changing functionality by replacing the macros or if the Macroable trait doesn't let you replace previously defined macros.
A better way might be to add a new BladeMacro class that maybe extends Str, or passes stuff on to it instead so that conflicts can't arise. It would also help with discoverability of your new methods because you could at least add them in the class level docblock as @method static string lcfirst($str): string.
@SeanJA Is there any package that you would use without reading the documentation? Of course, if it's totally automatic you would not dig deep, but when you use some package functionality manually, you need the docs. I think it's normal.
You are right, there can be "conflicts", but it's true not only for this package but for any package that provides Collection, Str, Blade, Request or Response macros. I don't think there is much stuff to do with it. That's why you need to read the docs, to see what are you using exactly. But it may be good to write it in the docs to watch out for macro conflicts.
I think to create a new class would be a little overkill, but it's true maybe documenting the macros a bit more would be nice. I will do these in a future commit.
Thank you!
@iamgergo After taking a look at the code in this package and making my pull request for this issue, I found that reading through the documentation gives no indication that you have overridden a couple of the methods already with the mb_ equivalents, you would have to read through the code carefully and be familiar with the contents of the Str class and the functionality of the Macroable trait to realise this.
@SeanJA First of all, I'm sorry. I really did not want to be rejective or "defensive". I was trying to cover my point of view, that's all. I was thinking on the PR that you made, and honestly, first I was more about rather delete the macros from the package than to have an extra class, but now I think it's a good approach. And I really appreciate that you made PR not only saying what should be there. Thank you!
@mikeerickson Again, I can just say, I did not intend to be rejective. For sure, after it was posted on Laravel News, quite much "attack" came, that I did not understand. It's not because the package is so good or something, it's just I don't get it, why people reacted the way they did since it's just a package, not a core feature. But for sure, it gave me the base stress to overreact.
Honestly, thank you, guys.