Closed RhysLees closed 2 years ago
Thanks for your quick PR (and all of the PRs you have been submitting lately!).
I think we can simplify this PR by moving the logic to a new accessor on the Package model (something like
getDisplayNameAttribute
) and potentially using Laravel's string helpers to remove the unwanted strings.This isn't tested but something like
$toRemove = [ 'nova 4', 'nova4', '(nova 4)', 'v4', 'for nova', 'for v4', "()", 'laravel nova', ]; Str::of($name)->remove($toRemove, false)->trim();
It would be good to have tests around this as well. I'm thinking of a test in
Tests\Unit\PackageTest
that uses a data provider to test for the following (plus any others you can think of):Package Name Is Converted To CKEditor4 CKEditor4 R64 Fields R64 Fields Nova ABC ABC Nova 4 ABC ABC Nova4 ABC ABC ABC for Nova v4 ABC ABC nova v4 ABC ABC for v4 ABC ABC (Nova 4) ABC ABC v4 ABC What do you think?
Yeah, ill give a test with this approach using Str helper but i still think we should probably use the section where it iterates through the version numbers from 1 to latest major so it will auto-extend the list when a new major version comes out. this will future and past proof the implementation.
That is a good point 👍🏾
That is a good point 👍🏾
Just pushed some updates, I noticed an issue caused by the order of the filter list where filters for newer versions were filtering text but not version numbers which caused failures so i swapped the loop around which fixed the issue: Old Generation
'For Laravel Nova 4',
'For Nova 4',
'N4',
'(4)', // Place at bottom
'For Laravel Nova 3',
'For Nova 3',
'N3',
'(3)', // Place at bottom
New Generation:
'For Laravel Nova 4',
'For Laravel Nova 3',
'For Nova 4',
'For Nova 3',
'N4',
'N3',
'(4)', // Place at bottom
'(3)', // Place at bottom
The only thing I can see being an annoyance is the order of the filter list in config being important for example putting !
at the top would mean Nova !
would never be filtered leaving Nova
but i suppose that is what the test is for
@marcusmoore I made them changes as requested btw.
Thanks @RhysLees. I haven't had a chance to check this out yet but plan on getting back to it later this week.
This PR filters unwanted information out of the package name.
Resolves #212