monero-ecosystem / monero-python

A comprehensive Python module for handling Monero cryptocurrency
BSD 3-Clause "New" or "Revised" License
246 stars 80 forks source link

Refactoring `backends` submodule #81

Closed jeffro256 closed 3 years ago

jeffro256 commented 3 years ago

Motivation: backends.jsonrpc was already probably longer than it needed to be. I split up the logic for JSONRPCDaemon and JSONRPCWallet in order to increase readibility. This will be especially important in the future as more backends are written. In particular, an issue (#73) is currently open which suggests adding MyMonero API support. If this is added, it might be worth also adding the OpenMonero API. Both of these are JSON RPC APIs, but I think it wouldn't necesarily make a lot of sense to put these all into the same file so I went ahead and split the backends up. Also, because of the default backends PR (#79), to the most basic of users, it will matter much less how the backends are named, if you chose to take issue with my naming. I hope this PR is useful for future development!

Changes:

  1. Moved JSONRPCDaemon to backends.rpcdaemon
  2. Moved JSONRPCWallet to backends.rpcwallet
  3. Moved jsonrpc exceptions to backends.rpcexceptions
  4. monero.backends.jsonrpc now only imports the backends and exceptions
  5. Edited test files accordingly

Notes:

jeffro256 commented 3 years ago

Sorry about all the PR confusion, git merges are hard apparently ;). This is the one I'm requesting at the moment.

emesik commented 3 years ago

Splitting the file is a good idea but you also have introduced backward-incompatible namespace change. All module users would have to modify their imports.

The backend coul still live under monero.backends.jsonrpc, even after the split. Additional backends could land in monero.backends.mymonero or monero.backends.openmonero, etc.

jeffro256 commented 3 years ago

Sorry if I do not understand but I'm 90% sure that this change is backwards compatible unless there is something I missed. Any library user can still import JSONRPCDaemon or JSONRPCWallet by doing the following:

from monero.backends.jsonrpc import JSONRPCDaemon, JSONRPCWallet

This is because jsonrpc.py now only contains the lines:

from .rpcdaemon import JSONRPCDaemon
from .rpcexceptions import RPCError, Unauthorized, MethodNotFound
from .rpcwallet import JSONRPCWallet
emesik commented 3 years ago

We may have two different approaches there:

  1. To just reduce the size of the file, the backend may go into a dir backends/jsonrpc/ which contains: __init__.py daemon.py (contains JSONRPCDaemon) wallet.py (contains JSONRPCWallet) exceptions.py (contains those 3 exceptions) Then __init__.py imports from internal files to keep structure and allow imports like they were before (100% backward-compatible).

  2. Actually split the backend in two, backends/jsonrpcdaemon.py and backends/jsonrpcwallet.py for the reason a backend may not neccesarily implement both daemon and wallet functionality (for example, mymonero/openmonero will not). Then provide some glue to keep backward-compatible imports.

IIUC, you have chosen the latter option. However, I find that solution a bit confusing and I think it pollutes namespace under backends/. Have I understood it correctly? Is it what you actually meant? Perhaps I'm missing something.

emesik commented 3 years ago

82 shows what I mean by variant 1.

jeffro256 commented 3 years ago

Yeah that seems to accomplish the same goal while being cleaner. I like it !

On Tue, Jan 19, 2021 at 19:16 Michał Sałaban notifications@github.com wrote:

82 https://github.com/monero-ecosystem/monero-python/pull/82 shows

what I mean by variant 1.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/monero-ecosystem/monero-python/pull/81#issuecomment-763256681, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACSWLOWX5HUZZAA2JEASZZTS2YVFPANCNFSM4WIMEVAA .

emesik commented 3 years ago

Good, I'm closing it then.