Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Initial Django inspired component loading #719

Merged
merged 8 commits into from
Nov 19, 2018
Merged

Initial Django inspired component loading #719

merged 8 commits into from
Nov 19, 2018

Conversation

ixje
Copy link
Member

@ixje ixje commented Nov 15, 2018

What current issue(s) does this address, or what feature is it adding?
A different approach for loading plugins as suggested in #699, Django inspired

How did you solve this problem?
add a class loader

How did you make sure your solution works?
manually run api_server.py with Rest and RPC servers

Are there any special changes in the code that we should be aware of?
Yes; the RPC_SERVER and REST_SERVER variables are now stored at module level and not part of the SettingsHolder class. The implication is that all networks (main/test/priv) use the same settings. The alternative is moving it into the SettingsHolder which means we need to update all protocol.xxx.json files to include new keys to load from and otherwise have a sane default.

What approach would be preferred here (requesting extra opinions 🙏 ): @metachris @jseagrave21 @ThomasLobker @dethos
choice 1: all networks have the same plugin loading settings
choice 2: all network can have individual plugin loading settings as defined in their protocol.xxx.json

Please check the following, if applicable:

  • Did you add any tests?
  • Did you run make lint?
  • Did you run make test?
  • Are you making a PR to a feature branch or development rather than master?
  • Did you add an entry to CHANGELOG.rst? (if not, please do)

@ixje
Copy link
Member Author

ixje commented Nov 15, 2018

PS: Don't merge yet. If this was the intended idea then I'll also have to update the documentation to remove any ExtendedJsonRpc references and make some note that there will be/is a plugin for it

@ixje ixje requested a review from localhuman November 15, 2018 09:00
@coveralls
Copy link

coveralls commented Nov 15, 2018

Coverage Status

Coverage decreased (-0.05%) to 83.768% when pulling 1836a60 on ixje:plugin_django_style into ec96ae7 on CityOfZion:development.

@jseagrave21
Copy link
Contributor

I may not fully understand the implications, but I think simpler is better. If we could make one change to update the mainnet, testnet, privnet settings between "default" and "extended", I think that is okay, since a user is usually only using one instance at time.

Either way, I think good documentation will allow for a user to operate the servers in the new way regardless of the implementation.

@localhuman
Copy link
Collaborator

Looks good! I think the only problem is that a user who installed from pip could not specify which plugin to use without editing the installed settings file.

I think choice 2 would allow for that. As long as we have sensible defaults for when plugins aren't specified in protocol.xxx.json we should be ok to allow for configuring plugins via that file.

ixje added 5 commits November 17, 2018 09:43
 into plugin_django_style

* 'plugin_django_style' of https://github.com/ixje/neo-python:
  correct privnet bootstrap name to avoid confusion
  Allow a raw transaction to be built without an active blockchain db in the environment (#718)
@ixje
Copy link
Member Author

ixje commented Nov 17, 2018

This should be it, I didn't see any documentation that referenced the ExtendedJsonRpc server.

Copy link
Collaborator

@localhuman localhuman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@jseagrave21
Copy link
Contributor

jseagrave21 commented Nov 17, 2018

PS: Don't merge yet. If this was the intended idea then I'll also have to update the documentation to remove any ExtendedJsonRpc references and make some note that there will be/is a plugin for it

@ixje Are you adding instructions in this PR on how to use the new setup? Or could you implement those instructions with #664?

@ixje
Copy link
Member Author

ixje commented Nov 19, 2018

For the time being I'm not adding the instructions with this PR. We should document this at a point though.

@ixje ixje merged commit 2a501a1 into CityOfZion:development Nov 19, 2018
@ixje ixje deleted the plugin_django_style branch November 19, 2018 10:18
@jseagrave21
Copy link
Contributor

If you like, I can incorporate instructions into #664.

jseagrave21 added a commit to jseagrave21/neo-python that referenced this pull request Nov 20, 2018
- revert changes to bring to parody with CityOfZion#719
jseagrave21 added a commit to jseagrave21/neo-python that referenced this pull request Nov 20, 2018
- update documentation for CityOfZion#712 and CityOfZion#719
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.

4 participants