jotyGill / openpyn-nordvpn

Easily connect to and switch between, OpenVPN servers hosted by NordVPN on Linux (+patch leakes)
GNU General Public License v3.0
628 stars 114 forks source link

A very big pull request :) #219

Closed 1951FDG closed 5 years ago

1951FDG commented 5 years ago

Hi There,

Since, I more or less finished the Android app, it was time to revisit the openpyn code !!

I was happy to see the use of logging, log files and all, also happy to see the use of exit code for the main functions (exactly what I was planning to implement for Android app usage), though it wasn't fully implemented and to make matters worse, status returns are hard to manage across code, so much so, that I decided that it was best to raise and handle exceptions, which, to some is the most pythonic thing to do apparently ...

One article I read was Exceptions vs. status returns

In the end the changes weren't that easy to make, and one downfall of exceptions in Python, is that while looking at a block of code, including functions which may or may not throw exceptions, there is no way to see which exceptions might be thrown and from where. Java at least has a way to annotate a function that could raise exceptions and inspection checks to check if an exception is not handled. Kotlin doesn't have exceptions !/? ....

Enfin, after much careful thinking, and I hope you can relate, the use of exceptions is better than the use of status returns (status returns are harder to manage with functions calling other functions, and also when testing functions for True, 1 is meant to be an error, as in not OK)

New Netflix servers

Added new servers according to support article

Function optimisation for router based hardware or slower CPUs, you won't notice on Desktop, but the netflix function uses a lot of CPU apparently...

Some Nord API changes (thanks to Android app I was working on, I discovered the API had changed):

"Dedicated IP servers" changed to "Dedicated IP" "Anti DDoS" changed to "Obfuscated Servers"

So, Anti DDoS option will now find Obfuscated Servers!

Added 'allow_abbrev=False' to all ArgumentParser**** This was new to me, but fixes bugs like:

-Status would be considered -s tatus, and would invoke server with strange text 'tatus'

So, with option 'allow_abbrev=False', the above statement would now lead to help message displayed, incorrect parameter usage

So, from now on, parameters have to be typed exactly as the code specifies

This will fix a ton of hidden bugs I believe...

'def run()' has been optimised

There were a couple of bugs while inspecting the code carefully and also a lot of reordering has been done, some minor optimisations to 'def connect()', since that code is run in a loop sometimes. Overall, some code was moved around, to simplify and to make testing of individual functions also "better"...

Forgot to mention, a lot of testing was done afterwards too! I also encourage you to test, and/or write more tests ...

jotyGill commented 5 years ago

Dude this is insane! how have you manage to spend so much effort into your android application (as well as openpyn). This is amazing! When I get a chance I'll review and merge this and hopefully work on more things/features. Respect

1951FDG commented 5 years ago

Thanks a lot, yeah it's been crazy, might have overdone it a bit, but I'm not done yet!!! Let me know when merged and/or when version is bumped, for I will enable version checking of openpyn in the android app, to make sure the app works as intended in the future... Thanks!