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

Huge changes #3

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Conversation

peter279k
Copy link
Member

@peter279k peter279k commented Apr 28, 2019

Changed log

  • Rename PHPImageConverter to ImageConverter class.
  • Using the PHPUnit to do unit tests.
  • Composer package ready.
  • Let this package require php-7.1 version at least.
  • Using the Travis CI build. The build log is available here.
  • Using php-coveralls to integrate the coveralls code coverage service. The code coverage report is here.
  • Remove the to folder because it's the output folder.
  • It's related to issue Better tests #1.

@jenstornell
Copy link
Collaborator

jenstornell commented Apr 28, 2019

@peter279k You call that "minor changes"? You have rewritten almost the whole thing. =)

Most of it looks good (without testing it), but I have a few questions about it.

Thanks!

@peter279k
Copy link
Member Author

@peter279k You call that "minor changes"? You have rewritten almost the whole thing. =)

Most of it looks good (without testing it), but I have a few questions about it.

  • What is the reason for using constants here?

This const value lists are for the exif_imagetype and this can check the image contents to determine the current image formats :).

  • Why a function to check for webp?

Actually, the webp image format cannot be recognized via exif_imagetype correctly if we use the php-5.6 version.

If we let package require php-7.1 version at least, this webp check method can be removed.

  • Why check for not webp here?

As the previous answer, if we let package require the php-7.1 version at least, this not webp condtion can be removed.

@peter279k peter279k changed the title Minor changes Huge changes Apr 28, 2019
@peter279k
Copy link
Member Author

@jenstornell, thank you for your reply.

I name the wrong title, and I will rename original to Huge changes.

I hope you can review them in details at your available time :).

@jenstornell
Copy link
Collaborator

@peter279k I will test it when I have more time.

PHP 7.1 was released december 2016 and I think it can be a requirement. It makes the code shorter and also a bit more readable because of that.

@peter279k
Copy link
Member Author

Hi @jenstornell, thank you for your reply.

I've removed the isWebpImage method because the package requires php-7.1 version at least.

@jenstornell jenstornell merged commit 40fe230 into free-open-source:master Apr 30, 2019
@peter279k peter279k self-assigned this Apr 30, 2019
@peter279k peter279k deleted the minor_chages branch April 30, 2019 17:59
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.

2 participants