Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naveenvalecha created an issue. See original summary.

ajalan065’s picture

Assigned: ajalan065 » naveenvalecha
Status: Active » Needs review
FileSize
8.48 KB

Here is the patch with the functions moved to services.

I have also checked the entire module for proper comments and whitespaces and added new lines to the EOF, where they were missing.

naveenvalecha’s picture

Assigned: naveenvalecha » ajalan065
Status: Needs review » Needs work
  1. +++ b/google_vision.services.yml
    @@ -1,4 +1,7 @@
    +  google_vision.fill_alt_text:
    

    Make the service name to be "google_vision.helper" and keep all the helper functions into it related to all detection types.

  2. +++ b/src/AutoAltTextFill.php
    @@ -0,0 +1,128 @@
    +class AutoAltTextFill {
    

    Class name to be GoogleVisionHelper Also define the interface as well. GoogleVisionHelperInterface

naveenvalecha’s picture

Uploaded the rerolled patch with suggestions above

naveenvalecha’s picture

Assigned: ajalan065 » Unassigned
Status: Needs review » Fixed

Committing and pushed to 8.x-1.x
While I was reviewing this I have also filed another issue #2783795: Define the Interface for GoogleVisionApi

eugene.ilyin’s picture

Status: Fixed » Active

Mhh, why do we need interface GoogleVisionHelperInterface.php? Is it standard?

naveenvalecha’s picture

Status: Active » Fixed

Yup this is made to keep in mind about the #2739855: Use code from https://github.com/ThijsFeryn/google-cloud-vision-api external library when we'll implement it later(i.e. when php7 will be requirement of the drupal).We will extend the interfaces of that library in our own interface and we'll not break the constructors of our own services(api) as per the d.o. B.C. policy

‘Holy Grail’ of programming is the reuse of existing code – interfaces play an important role in this.

Feel free to reopen if you need any more clarity on it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.