botman / driver-facebook

BotMan Facebook Messenger Driver
MIT License
71 stars 73 forks source link

Add I/O type hinting, remove duplicate code, improve Docblock, add some optimizations #84

Closed ker0x closed 5 years ago

ker0x commented 6 years ago

This PR do:

Warning

If this PR is merged, the next release should be tag 2.0 as it bring I/O type hinting. By doing so, this will avoid BC break for people using version < 2.0

codecov[bot] commented 6 years ago

Codecov Report

Merging #84 into master will decrease coverage by 0.61%. The diff coverage is 74.31%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #84      +/-   ##
============================================
- Coverage     71.17%   70.55%   -0.62%     
+ Complexity      477      429      -48     
============================================
  Files            49       49              
  Lines          1332     1304      -28     
============================================
- Hits            948      920      -28     
  Misses          384      384
Impacted Files Coverage Δ Complexity Δ
src/Events/FacebookEvent.php 60% <ø> (ø) 2 <0> (ø) :arrow_down:
src/Commands/Nlp.php 0% <0%> (ø) 4 <0> (ø) :arrow_down:
src/Commands/AddPersistentMenu.php 0% <0%> (ø) 4 <0> (ø) :arrow_down:
src/Commands/WhitelistDomains.php 0% <0%> (ø) 4 <0> (ø) :arrow_down:
src/Commands/AddGreetingText.php 0% <0%> (ø) 4 <0> (ø) :arrow_down:
src/Events/MessagingPostbacks.php 0% <0%> (ø) 1 <1> (ø) :arrow_down:
src/Providers/FacebookServiceProvider.php 0% <0%> (ø) 5 <1> (ø) :arrow_down:
src/Commands/AddStartButtonPayload.php 0% <0%> (ø) 4 <0> (ø) :arrow_down:
src/Events/MessagingDeliveries.php 100% <100%> (ø) 1 <1> (ø) :arrow_down:
src/Events/MessagingOptins.php 100% <100%> (ø) 1 <1> (ø) :arrow_down:
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 627d1b8...0d4aae8. Read the comment docs.

christophrumpel commented 5 years ago

Hey @ker0x, thanks for the PR.

We like the docblocks on one line if possible like /** @var ... **/. The return types addition is nice, but we would want to change that in all repositories which is a lot of work. We also want to that together when we change. So I can't merge this PR now.

If you could change the docblock onliners back and remove the return types. We can merge this now and later the return types.

ker0x commented 5 years ago

Hello @christophrumpel,

Thank you for your feedback. I've closed this PR due to problems with my master branch and open a new one with changes that you requested (#87).