You are viewing a single comment's thread from:

RE: Tasks API implementation into the PHP Client for Makerlog

in #utopian-io • 6 years ago

Thank you for your contribution.

  1. The build is failing on the github page.
  2. It would be nice to have some unit tests to cover the new classes you added.
  3. The refresh() is actually clearing the caching (doesn't do what exactly it says), it would be better to move (or re-organise) the correct logics e.g the code in getTaskdata into this function.
  4. Often, the chainning such as $this->Makerlog->getRequest()->get('/tasks/sync/'); is a code smell, because something may be null in the middle, try to have if null check, then throw exception accordingly.

Your contribution has been evaluated according to Utopian policies and guidelines, as well as a predefined set of questions pertaining to the category.

To view those questions and the relevant answers related to your post, click here.

Need help? Chat with us on Discord.

[utopian-moderator]

6 years ago in #utopian-io by
$12.66
  • Past Payouts $12.66
  • - Author $9.62
  • - Curators $3.04
22 votes
Reply 2
Sort:  
  • Trending
    • [-]
     6 years ago (edited)

    The build is failing on the github page.

    yes, there is an issue for that. it's an issue with travis-ci and i have no solution at the moment :(

    The refresh() is actually clearing the caching (doesn't do what exactly it says), it would be better to move (or re-organise) the correct logics e.g the code in getTaskdata into this function.

    good argument. I will take to heart
    oh, and thanks for the improvement suggestions

    $0.00
      Reply
      [-]
       6 years ago 

      Thank you for your review, @justyy! Keep up the good work!

      $0.00
        Reply