Skip to content
This repository has been archived by the owner on Mar 9, 2024. It is now read-only.

Refactoring backends submodule #81

Closed
wants to merge 2 commits into from
Closed

Refactoring backends submodule #81

wants to merge 2 commits into from

Conversation

jeffro256
Copy link
Contributor

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:
* This change is backwards compatible

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:
    * This change is backwards compatible
@jeffro256
Copy link
Contributor Author

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

@jeffro256 jeffro256 mentioned this pull request Jan 19, 2021
@emesik
Copy link
Contributor

emesik commented Jan 19, 2021

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
Copy link
Contributor Author

jeffro256 commented Jan 19, 2021

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
Copy link
Contributor

emesik commented Jan 20, 2021

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
Copy link
Contributor

emesik commented Jan 20, 2021

#82 shows what I mean by variant 1.

@jeffro256
Copy link
Contributor Author

jeffro256 commented Jan 20, 2021 via email

@emesik
Copy link
Contributor

emesik commented Jan 20, 2021

Good, I'm closing it then.

@emesik emesik closed this Jan 20, 2021
@jeffro256 jeffro256 deleted the backend_refactor branch January 22, 2021 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants