search

skorokithakis / A6lib

An ESP8266/Arduino library for communicating with the A6 GSM module. ⛺
MIT License
127 stars 49 forks source link

Improvements and so on #34

Closed IMAN4K closed 6 years ago

IMAN4K commented 6 years ago

Signed-off-by: Iman Ahmadvand iman72411@gmail.com (github: @IMAN4K)

I just didn't touch Dial section of code base, but will do it in coming days. And GPRS & IP APIs should be added

skorokithakis commented 6 years ago

Oh man, this is huge. Can you split it into multiple smaller PRs? It will take me ages to review like this, and I can't really test it :/

IMAN4K commented 6 years ago

The PRs are already split :) I suggest creating new dev branch to adding + reviewing changes step by step then merge to master as new release. BTW i have tested with ESP8266 in last 25 days! with these two modules 1 and 2

Regards. IMAN.

skorokithakis commented 6 years ago

What do you mean "already split"? It's one large PR, isn't it? I'm also not sure what you mean by creating a dev branch...

IMAN4K commented 6 years ago

I mean there are 8 commit in this PR so you can easily review changes in each commit right? I prefer not to put all changes directly to master, for that you must create new branch named dev in addition to master and we put changes in dev first And BTW the code style is pretty fine, you shouldn't have problem review changes!

That's all.

skorokithakis commented 6 years ago

I can review the changes, but I can't merge one commit that is fine and not merge another one that I might have feedback on, that's why I prefer separate PRs. Also, there are some conflicts that need to be resolved.