search

Jamie- / openvpn-api

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

Improvements in code #4

Closed HosseyNJF closed 4 years ago

HosseyNJF commented 4 years ago

I added some improvements to the code. The code was less or more like spaghetti code, So I splitted parts of the code to multiple classes, like Connection.

I also added a base model for all models (now and future), to easily be able to plug in new models.

D0han commented 4 years ago

Hi!

  1. This is a draft, are you working on something else in this PR?
  2. Proposed changes touch pretty much every single area of code, so working on anything at all in parallel will be impossible due to conflicts.
  3. Interface of package has been changed, this could be problematic.

@Jamie- please take a look

D0han commented 4 years ago

Also, this PR is pretty big, it makes it hard to review properly. Can you split it into separate PRs with clear goal description?

HosseyNJF commented 4 years ago
  1. This is a draft, are you working on something else in this PR?

I'm adding a multi-threaded system of sockets to be able to work with OpenVPN's events, because that's what I need in my VPN service, and I thought why not contribute to an already existing project :)

  1. Proposed changes touch pretty much every single area of code, so working on anything at all in parallel will be impossible due to conflicts.
  2. Interface of package has been changed, this could be problematic.

Of course, If this pull request (and next commits about the event system) is going to be merged, It's going to be a major version upgrade and backward compatibility is probably not a problem.

HosseyNJF commented 4 years ago

Also, this PR is pretty big, it makes it hard to review properly. Can you split it into separate PRs with clear goal description?

I think it's not possible because each commit of this PR depends directly on the previous one, They're not isolated or separate.

HosseyNJF commented 4 years ago

@Jamie- Actually I was inspired by Django to merge all models into one file, why that's not acceptable?

HosseyNJF commented 4 years ago

I don't know how I could split my already committed changes into multiple PRs, but yeah I also agree splitting would be better.

Also, I'm already developing the event system (in the events branch in my fork) using the same socket module and multiple threads. I would be happy If you take a look at it (actually I haven't pushed all of my changes; I will tomorrow).

D0han commented 4 years ago

I guess you could try to do interactive rebase to split commits that are too big, and then create branches with smaller changes.

HosseyNJF commented 4 years ago

I'll cherry pick commits from my master to multiple branches, and create PRs for each branch. You guys should ignore this PR and focus on the others.