Electron-Cash / Electron-Cash

Electron Cash; Bitcoin Cash thin client
MIT License
364 stars 191 forks source link

Consider moving all code into a top-level package #807

Closed mhsmith closed 3 years ago

mhsmith commented 6 years ago

Electrum has now done this. As per the current setup.py, I assume we'd want our package to be called electroncash rather than electrum, otherwise it would be impossible to install both programs in the same environment. This will unfortunately make merges from Electrum harder than they were before, but no harder than they are right now since Electrum made this change.

Advantages:

Disadvantages:

cculianu commented 6 years ago

I admit it's rather bizarre the way the dirs are mapped to module names right now.

I fear this may create a subtle source of bugs unless we are careful with text searches (I guess it shouldn't be too bad.. but still).

So no more "electroncash_gui" namespace? Everything is filed under "electroncash" now? (Eg electroncash.gui)??

This external plugins will also be affected... :/

Are you sure you want to open this can of worms? :P

(Clearly I'm of the pragmatic mindset: if it ain't broke, don't fix it.)

PS: Right now might not be a good time. There is a parallel fork of Electron Cash called "SLP" that aims to add tokens to Electron Cash. Perhaps wait until that's done?

fyookball commented 6 years ago

i'd rather avoid this if not necessary

mhsmith commented 6 years ago

Fair enough: it was mentioned on Slack a couple of days ago, so I thought it should be recorded somewhere more permanent. The workaround for Chaquopy had a small performance cost, but I've already solved that for another project so it shouldn't be a factor anymore.

Let's leave it open for another day or so in case anyone else wants to comment.

kyuupichan commented 6 years ago

I think this is a good idea. Electron Cash now warns it's using obsolete features with Python3.7 because of the current hack

rt121212121 commented 6 years ago

Some things we need to address:

For example: https://github.com/spesmilo/electrum/commits/master/electrum/network.py

On Fri, Jul 20, 2018 at 1:01 PM Neil notifications@github.com wrote:

I think this is a good idea. Electron Cash now warns it's using obsolete features with Python3.7 because of the current hack

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fyookball/electrum/issues/807#issuecomment-406457884, or mute the thread https://github.com/notifications/unsubscribe-auth/AfFyJrn-bgnS-A2-vb4byqauHSI0Ey3nks5uISv3gaJpZM4VWGKW .

rt121212121 commented 6 years ago

Here's the electrum commit, which would be a good point of reference to ensure nothing is missed.

https://github.com/spesmilo/electrum/commit/097ac144d976eb46dff809e1809783dc78ab6d8b#diff-554a6e574ded9f74c5821ea55e34d488

cculianu commented 3 years ago

Ok, this wasn't quote handled fully by #2020 but partially. The reason why it wasn't done completely is that it's a lot smaller a diff to do it the way it was done in #2020. Ideally we'd have electroncas/gui and electroncash/plugins but that would break existing external plugins -- and maybe that would be a major revision thing.

mhsmith commented 3 years ago

Yeah, I don't think that would be worth the trouble.