search

Jamie- / openvpn-api

A Python API for the OpenVPN management interface.
MIT License
60 stars 18 forks source link

Refactor socket connection logic into separate class #6

Closed HosseyNJF closed 4 years ago

HosseyNJF commented 4 years ago

In order to have an extendable library (to be able to add events, etc.), business logic for every part of code should be separated in their own class and files. I have put the socket connection logic in another completely isolated class.

HosseyNJF commented 4 years ago

Do me a favor by not looking at the commits themselves and only seeing diff of the PR :) I made some weird, unwanted bugs while committing those first commits

HosseyNJF commented 4 years ago

@Jamie- It would be great to have a dev branch for these PRs to merge.

HosseyNJF commented 4 years ago

Hmm.. could Connection instance be configured to make actual connections only when its needed?

What if the client was sending requests with rate of like 50 requests per second? That would perform really bad. But giving this option to user (connect-on-demand) is a good idea as long as they are not using events. It could be part of another PR; This one is focused on just adding the Connection class.

D0han commented 4 years ago

What if the client was sending requests with rate of like 50 requests per second? That would perform really bad.

Yes, that is why i asked about configuring it.

Jamie- commented 4 years ago

@Jamie- It would be great to have a dev branch for these PRs to merge.

~I've created a dev branch just now. Looking through these changes~ I changed my mind, let's do development in master and handle releases in a releases/x.x.x branch. I've bumped the version to 1.0.0

Jamie- commented 4 years ago

I understand the goal of being able to handle events and trigger callbacks when the socket sends a message to us. But I don't entirely see the point in splitting VPN out into Connection and VPN. I can't see how this is gaining anything.

Equally I understand you'd like things segregated into separate classes/modules as sure, different things have different purposes, but even after splitting, Connection and VPN are still heavily coupled. I think a perhaps better approach is to split out all of the parsing of the data returned from the socket. That I've already started to do if you have a look in the new dev branch (previously was in a parsing branch).

Seems like the interface exposed has just become more complex to use. I might be missing something so I'll look at the next PR to see what it builds on and come back to this.

HosseyNJF commented 4 years ago

I'm thinking about it and I think you're right. After doing the job, it doesn't feel good because the classes are tightly coupled to each other and there is really no point in it. I'll close this PR and open another one for my event system, rebased from the current master.