binshops / prestashop-rest

PrestaShop REST API module to expose your PrestaShop website's REST endpoints
https://www.binshops.com/prestashop-api
Academic Free License v3.0
95 stars 33 forks source link

Multiple improvements #39

Closed alisamie97 closed 1 year ago

alisamie97 commented 1 year ago
  1. Composer autoload
  2. register hooks in one method
  3. create tables while installing to show db errors if any exists
  4. remove some unused methods and codes
  5. upgrade postman collection with some random dynamic variables
samberrry commented 1 year ago

Nice PR, I will review it, thanks for it ;)

samberrry commented 1 year ago

Hey @stifler97, I reviewed your PR, here are the details:

0- try to push your PR on develop branch, just rebase your current branch with develop

1- adding composer is a nice addition, it looks okay to me, and "prepend-autoloader": false is set. We just need consider namespaces, why? to avoid future conflicts, (for example, there is a probability of having a class name somewhere in the whole project with the same name RESTTrait.php in the future)

how:

same standard for other classes and controllers.

Note: As the composer and the namespaces need to be checked carefully, we can better track it through another dedicated PR for this feature.

2- register hooks in one method, looks okay to me.

3- It's not necessary, we can keep the old version.

4- We can keep these kind of codes, because this module is for developers and we usually use it as a base module, sometimes we use these lines to build up forms with a quick copy and paste, so they can be used as a boilerplate code.

5- Not necessary to change website_url to SHOP_URL, keep it website_url and I'm not sure about Random variables, because people usually want to see the result of the fixed sent request, especially random password does not make sense. But changing sam@binshops.com to something else like test@mail.com and names to John, Doe would be good.

samberrry commented 1 year ago

Just to know, this PR's changes are not merged, I just force pushed the older version of develop. If you want to work on it you must create a new PR.