search

yassinedoghri / php-icons

A php library based on iconify's API to download and render svg icons from popular open source icon sets.
MIT License
6 stars 3 forks source link

PEST + Contributing docs (preparing v1.0) #2

Closed benjaminhaeberli closed 1 month ago

benjaminhaeberli commented 1 month ago

Subject:

Description:

Here is my first PR to add some contributing docs, add PEST ^3.0 (which requires PHP >=8.2) and a security policy.

Related:

Discussed in #1

benjaminhaeberli commented 1 month ago

@yassinedoghri I made all the changes, however when I run composer style:fix, nothing changes. I usually use laravel/pint and not easy-coding-standard, can you have a look, or maybe we could switch to pint ?

yassinedoghri commented 1 month ago

@benjaminhaeberli I've just applied the fix! I ran composer style:fix, not sure why it didn't work for you :thinking:

I didn't know about https://github.com/laravel/pint, looks nice!

yassinedoghri commented 1 month ago

I've reset the php version to 8.2 for the quality action. Now, there's a phpstan error, I'll leave you to fix it.

I think the rest is fine by me :slightly_smiling_face:

benjaminhaeberli commented 1 month ago

Cool ! I'll fix it today, thanks a lot @yassinedoghri !

yassinedoghri commented 1 month ago

Looks like the phpstan error should just be ignored as it's a known bug in pest: https://github.com/pestphp/pest/issues/997

I'll add a rule to ignore it!

yassinedoghri commented 1 month ago

Ok, I stop messing with your PR now :smile:

I'll leave you to it with pest! Thank you for the changes :)

benjaminhaeberli commented 1 month ago

Haha no problem ! I didn't have time yesterday, I'll check ASAP :)

yassinedoghri commented 1 month ago

No worries, take your time! :+1:

benjaminhaeberli commented 1 month ago

Here it is @yassinedoghri ! It's very basic and I'm sure it could be improved but it's a starting point. I never used Ahc\Cli before so maybe there is a better way to write tests for it 🤷‍♂️ I didn't use mocking to avoid too much complexity and start simple, what do you think ?

yassinedoghri commented 1 month ago

[...] I didn't use mocking to avoid too much complexity and start simple, what do you think ?

Looks good to me, thanks! :slightly_smiling_face:

Apparently there's an issue with the tests: looks like the Icons::DATA array is empty on first execution... Wondering if it's because DATA is a const :thinking:

I'll check this out tomorrow, let me know if you have an idea!

benjaminhaeberli commented 1 month ago

Apparently there's an issue with the tests: looks like the Icons::DATA array is empty on first execution... Wondering if it's because DATA is a const 🤔

Yes I even tried to add a sleep(5) before the expectation, but it doesn't work 🧐

Suggestion : This would require a bit of refactoring, but wouldn't it be simpler to abandon the Icons.php file and store the icons as svg files in src/icons, using a prefix, for example: lucide_icon-name.svg?

yassinedoghri commented 1 month ago

Yes I even tried to add a sleep(5) before the expectation, but it doesn't work 🧐

Yeah, not sure what's happening there TBH!

Suggestion : This would require a bit of refactoring, but wouldn't it be simpler to abandon the Icons.php file and store the icons as svg files in src/icons, using a prefix, for example: lucide_icon-name.svg?

I have started out the project with storing icons in an icon directory but quickly came to some limitations.\ Had the idea of having them in a class constant to allow for easier diffing from the get go (to check which icons have been added / edited / removed and show stats to the user in the CLI tool). Moreover, I believe it is quicker to get icons this way as file IO is usually slower. It's a belief though, can't back it up unless doing a benchmark + it may not even be a question in regards to the number of icons used in projects (I'm guessing a few dozens to hundreds at most). And in practice, it has worked pretty well thus far on adaures/castopod with 158+ icons.

Anyways, I've come up with a solution for the tests by checking the contents of the Icons.php file. I think it's a good start, let me know if you have any comment :slightly_smiling_face:

I think I can merge if everything is ok on your side, thank you for helping out! :pray:

benjaminhaeberli commented 1 month ago

Anyways, I've come up with a solution for the tests by checking the contents of the Icons.php file. I think it's a good start, let me know if you have any comment 🙂

I just had a look at your commit and it looks very good, Iet's merge and release a v1.0 !

I think I can merge if everything is ok on your side, thank you for helping out! 🙏

You're welcome! Working together on this is fun😉

yassinedoghri commented 1 month ago

I just had a look at your commit and it looks very good, Iet's merge and release a v1.0 !

Great, thanks! I'll merge tomorrow and work on the v1.0 release this weekend :)

github-actions[bot] commented 1 month ago

:tada: This PR is included in version 1.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: