[WIP] split the client from the cli (take 2) #49
Conversation
| if not data_package_path: | ||
| data_package_path = os.getcwd() | ||
| data_package_path = os.path.abspath(data_package_path) | ||
| # may want to use the datapackage-py here | ||
| self.datapackage = self._load_dp(data_package_path) | ||
|
|
||
| # load config ... | ||
| self.config = self._load_config(config_path) | ||
| try: |
There was a problem hiding this comment.
I don't think we need to require these things simply to boot the client - imagine i'm just trying to validate the package without a username and password?
There was a problem hiding this comment.
Generally i think we just want in the __init__:
self.config = config
Then somewhere where we need config info - e.g. in publish or other methods we can use a self._ensure_config or whatever (probably in ensure_auth etc).
There was a problem hiding this comment.
I'd suggest to split out the local validate command from client as the top-level function. Client could reuse that inside, for ex. validating prior to publising.
So the client will honor its name better, being abstraction of the REST-endpoint. This could decrease the level of API surprises.
There was a problem hiding this comment.
@Fak3 can we just do the simple thing here I've suggested. we don't want validate split out.
| class Client(object): | ||
|
|
||
| def __init__(self, data_package_path='', config_path="~/.dpm/config", click=None): | ||
| def __init__(self, config, data_package_path='', click=None): |
There was a problem hiding this comment.
data_package_path should come first - it is primary.
Re config: i am in two minds as to whether we want client to create config object or get passed it. From point of view of a library using this client i guess passing client in may be nice - it can also be mocked out so this way is nicer 😄
There was a problem hiding this comment.
Thinking more on the passing it datapackage in the constructor:
We will have install command at some point (#30). It may be useful to install multiple packages like npm\pip: dpmpy install @pub/pak1 @pub/pak2 etc... With current approach, bounding the client to a single dp - the cli will have to instantinate a new client for each package. This seems like one surprising API design. (Looking at any other client-like API in the wild - those usually focus on abstracting away the rest endpoint with more convenient wrappers)
The same applies to the deletion of multiple packages.
And I could imagine the command to list all your published packages on the server - it does not require a datapackage instance at all.
So I would suggest passing the datapackage to the actual method of the client, where it is needed. The multiple deletion will then look like this:
client = Client()
for package_id in packages:
client.delete(package_id)
There was a problem hiding this comment.
And possibly even more useful command: search a datapackage on the registry server. That is given some text string perform full-text search on the indexed fields and return a list of matched packages. This command does not require a datapackage either.
There was a problem hiding this comment.
search a datapackage on the registry server
Searching from the client is also part of the scope for the project, so yes.
There was a problem hiding this comment.
@Fak3 we do not want to pass the datapackage in to the client. It should create it.
We can think about more complex refactor late but for right now i think this makes sense.
We want the "cli" part to be as "dumb" as possible.
| 'password': 'password', | ||
| 'server': 'http://127.0.0.1:5000' | ||
| } | ||
|
|
||
| def test___init__fails_no_data_package(self): |
There was a problem hiding this comment.
See comments: above. Should be able ot initiate of config = None.
| def test__init__config_ok(self): | ||
| pass | ||
|
|
||
| def test__init__config_password_missing(self): |
There was a problem hiding this comment.
This stuff should come up when we actually need the config.
|
|
||
| class DpmException(Exception): | ||
| pass | ||
|
|
||
| class NetworkError(Exception): |
There was a problem hiding this comment.
Personally, i think we could just have a single DpmException and just change the message. We don't need to distinguish types of error for us at the class level.
And if we are going to have different types we should inherit from DpmException.
There was a problem hiding this comment.
Having only a single exception will give callers a headache to handle properly. On the single base exception - yes, that may be useful.
| idx, resource.name, resource.local_data_path) | ||
| ) | ||
|
|
||
| return True | ||
|
|
||
| def ensure_auth(self): |
There was a problem hiding this comment.
In a perfect world this should be a "private" method so _ensure_auth. However, i don't think this matters for now so let's leave as is.
|
|
||
| response = methods.get(method)(*args, headers=headers, **kwargs) | ||
|
|
||
| try: |
There was a problem hiding this comment.
Why do we need to wrap this. Why can't we just do response.json()? If it barfs let's handle higher up. It avoids tedious re-wrapping of errors like this.
There was a problem hiding this comment.
Because the python2 throws very generic ValueError on the malformed json. Caller of this method will have hard time to handle exception properly as ValueError could be raised from somewhere else, like encoding what he gave us as parameters.
| @@ -13,10 +13,6 @@ | |||
| class DpmException(Exception): | |||
There was a problem hiding this comment.
@Fak3 one small comment i'm making here: can we get more descriptive commits e.g. here you also moved config around. Also more smaller commits would be easier to review.
Addreses #43