Initial work on splitting bitmessagemain.py into multiple files #238

2013-06-21T01:32:39+02:00

DivineOmega commented

2013-06-21 01:32:39 +02:00

(Migrated from github.com)

Most code being in the single main file (bitmessagemain.py) was raised as being a possible issue for maintainability in the future - see issues https://github.com/Bitmessage/PyBitmessage/issues/235 and https://github.com/Bitmessage/PyBitmessage/issues/200.

This pull request shows my work so far towards splitting up the main file a bit. I'm finding it a little tricky and I'd really like to hear people's opinions/feedback on this.

Most code being in the single main file (bitmessagemain.py) was raised as being a possible issue for maintainability in the future - see issues https://github.com/Bitmessage/PyBitmessage/issues/235 and https://github.com/Bitmessage/PyBitmessage/issues/200. This pull request shows my work so far towards splitting up the main file a bit. I'm finding it a little tricky and I'd really like to hear people's opinions/feedback on this.

fiatflux commented

2013-06-21 18:18:21 +02:00

(Migrated from github.com)

Thanks for working on this! It will be much easier to maintain this code after the changes you're starting. In the meantime, I am nervous about major conflicts between this and pull/234 (& pull/236). Our changes are mostly basic gruntwork on bitmessagemain.py, but they already touch a lot of the file.

Since your changes are fairly modular and straightforward to redo on our changes, I'm thinking that the path of least resistance is:

Start with pull/236 (commit/c1a30d3, say).
Manually "replay" your changes on the logger-ized code.
Finish multi-file refactor.
Finish transition to logger.

What do you think of this plan?

Of course, if we had planned ahead it would have made more sense to work the other way around, starting with your changes. But alas, here we are.

Thanks for working on this! It will be much easier to maintain this code after the changes you're starting. In the meantime, I am nervous about major conflicts between this and pull/234 (& pull/236). Our changes are mostly basic gruntwork on bitmessagemain.py, but they already touch a lot of the file. Since your changes are fairly modular and straightforward to redo on our changes, I'm thinking that the path of least resistance is: - [ ] Start with pull/236 (commit/c1a30d3, say). - [ ] Manually "replay" your changes on the logger-ized code. - [ ] Finish multi-file refactor. - [ ] Finish transition to logger. What do you think of this plan? Of course, if we had planned ahead it would have made more sense to work the other way around, starting with your changes. But alas, here we are.

DivineOmega commented

2013-06-21 18:45:14 +02:00

(Migrated from github.com)

I'm more experienced using SVN that Git, so I'm not all too certain of its merging capabilities. It'll be interested to see either way.

Logically, I would have to agree with @Atheros1 that merging this pull first should be less error prone (and I believe it is currently in a state that causes no regressions, based on my testing). However, I respect this may give @fiatflux and @xj9 more work dependant on how well Git performs its merging.

Regardless, I'm happy to go with either way, and wish to defer the decision to those with more Git experience than myself.

I'm more experienced using SVN that Git, so I'm not all too certain of its merging capabilities. It'll be interested to see either way. Logically, I would have to agree with @Atheros1 that merging this pull first should be less error prone (and I believe it is currently in a state that causes no regressions, based on my testing). However, I respect this may give @fiatflux and @xj9 more work dependant on how well Git performs its merging. Regardless, I'm happy to go with either way, and wish to defer the decision to those with more Git experience than myself.

fiatflux commented

2013-06-21 18:55:38 +02:00

(Migrated from github.com)

I'm fine either way, but am interested to hear what @xj9 thinks. They seem
to be rather git-adept.

Even though I don't necessarily agree that merging pull/238 first is less
error-prone, merging this first has
PROS:
Leaves a reference version in the main fork that should behave to users
just the same as the current one.
CONS:
More aggregate work for people involved.

On Fri, Jun 21, 2013 at 10:45 AM, DivineOmega notifications@github.comwrote:

I'm more experienced using SVN that Git, so I'm not all too certain of its
merging capabilities. It'll be interested to see either way.

Logically, I would have to agree with @Atheros1https://github.com/Atheros1that merging this pull first should be less error prone (and I believe it
is currently in a state that causes no regressions, based on my testing).
However, I respect this may give @fiatflux https://github.com/fiatfluxand
@xj9 https://github.com/xj9 more work dependant on how well Git
performs its merging.

Regardless, I'm happy to go with either way, and wish to defer the
decision to those with more Git experience than myself.

—
Reply to this email directly or view it on GitHubhttps://github.com/Bitmessage/PyBitmessage/pull/238#issuecomment-19827032
.

I'm fine either way, but am interested to hear what @xj9 thinks. They seem to be rather git-adept. Even though I don't necessarily agree that merging pull/238 first is less error-prone, merging this first has PROS: Leaves a reference version in the main fork that should behave to users just the same as the current one. CONS: More aggregate work for people involved. On Fri, Jun 21, 2013 at 10:45 AM, DivineOmega notifications@github.comwrote: > I'm more experienced using SVN that Git, so I'm not all too certain of its > merging capabilities. It'll be interested to see either way. > > Logically, I would have to agree with @Atheros1https://github.com/Atheros1that merging this pull first should be less error prone (and I believe it > is currently in a state that causes no regressions, based on my testing). > However, I respect this may give @fiatflux https://github.com/fiatfluxand > @xj9 https://github.com/xj9 more work dependant on how well Git > performs its merging. > > Regardless, I'm happy to go with either way, and wish to defer the > decision to those with more Git experience than myself. > > — > Reply to this email directly or view it on GitHubhttps://github.com/Bitmessage/PyBitmessage/pull/238#issuecomment-19827032 > .

Atheros1 commented

2013-06-21 19:30:15 +02:00

(Migrated from github.com)

On my local machine I just merged #238 and then #236 and it actually went very smoothly (few conflicts). It appears that all or most of your logger-changes have been to things that weren't migrated to other files. So I will go about testing #238 by itself and will then merge into master and then I'll manually merge in #236.

fiatflux commented

2013-06-21 22:26:01 +02:00

(Migrated from github.com)

Oh, cool, glad to hear it wasn't too bad. Sounds like a good plan. I'll
still hold off on more logger conversions until main gets broken into
manageable chunks. (Especially easy since I'll be busy for several days.)

On Fri, Jun 21, 2013 at 11:30 AM, Jonathan Warren
notifications@github.comwrote:

On my local machine I just merged #238 https://github.com/Bitmessage/PyBitmessage/issues/238and then
#236 https://github.com/Bitmessage/PyBitmessage/issues/236 and it
actually went very smoothly. It appears that all or most of your
logger-changes have been to things that weren't migrated to other files. So
I will go about testing #238 https://github.com/Bitmessage/PyBitmessage/issues/238by itself and will then merge into master and then I'll manually merge in
#236 https://github.com/Bitmessage/PyBitmessage/issues/236.

—
Reply to this email directly or view it on GitHubhttps://github.com/Bitmessage/PyBitmessage/pull/238#issuecomment-19829527
.

Oh, cool, glad to hear it wasn't too bad. Sounds like a good plan. I'll still hold off on more logger conversions until main gets broken into manageable chunks. (Especially easy since I'll be busy for several days.) On Fri, Jun 21, 2013 at 11:30 AM, Jonathan Warren notifications@github.comwrote: > On my local machine I just merged #238https://github.com/Bitmessage/PyBitmessage/issues/238and then > #236 https://github.com/Bitmessage/PyBitmessage/issues/236 and it > actually went very smoothly. It appears that all or most of your > logger-changes have been to things that weren't migrated to other files. So > I will go about testing #238https://github.com/Bitmessage/PyBitmessage/issues/238by itself and will then merge into master and then I'll manually merge in > #236 https://github.com/Bitmessage/PyBitmessage/issues/236. > > — > Reply to this email directly or view it on GitHubhttps://github.com/Bitmessage/PyBitmessage/pull/238#issuecomment-19829527 > .

DivineOmega commented

2013-06-21 23:44:27 +02:00

(Migrated from github.com)

@Atheros1 Thanks for fixing some of the import inconsistencies.

I'm curious why you decided to move the singleListener class back into the bitmessagemain file. I'd like to understand the decision and check if I missed anything important.

@Atheros1 Thanks for fixing some of the import inconsistencies. I'm curious why you decided to move the singleListener class back into the bitmessagemain file. I'd like to understand the decision and check if I missed anything important.

Atheros1 commented

2013-06-21 23:56:54 +02:00

(Migrated from github.com)

The singleListener class references the sendDataThread and the ReceiveDataThread classes. When it was in its own file it couldn't reference those two classes. We could do:

import bitmessagemain

..but I feel like doing that sort of thing defeats the purpose of modularizing the code. If you think using import bitmessagemain is best then we can do that.

The singleListener class references the sendDataThread and the ReceiveDataThread classes. When it was in its own file it couldn't reference those two classes. We could do: ``` import bitmessagemain ``` ..but I feel like doing that sort of thing defeats the purpose of modularizing the code. If you think using import bitmessagemain is best then we can do that.

DivineOmega commented

2013-06-22 00:09:27 +02:00

(Migrated from github.com)

Ah, my bad. That issue did not come up in my testing as I have not set up any port forwarding, thus no incoming connections ever occurred.

I believe for the sake of splitting up the code (at least initially), we should 'import bitmessagemain'. I agree that it's probably the best we can do for now.

However, I was considering moving the sendDataThread and receiveDataThread classes into their own files, which we could then import, thus removing (or at least reducing) the need to import bitmessagemain. What do you think?

Ah, my bad. That issue did not come up in my testing as I have not set up any port forwarding, thus no incoming connections ever occurred. I believe for the sake of splitting up the code (at least initially), we should 'import bitmessagemain'. I agree that it's probably the best we can do for now. However, I was considering moving the sendDataThread and receiveDataThread classes into their own files, which we could then import, thus removing (or at least reducing) the need to import bitmessagemain. What do you think?

Atheros1 commented

2013-06-22 00:13:03 +02:00

(Migrated from github.com)