Skip to content

Adding interface class and local interface implementation#90

Merged
phil-brad merged 6 commits intomasterfrom
spawn-interface
Feb 4, 2019
Merged

Adding interface class and local interface implementation#90
phil-brad merged 6 commits intomasterfrom
spawn-interface

Conversation

@mtinning
Copy link
Member

Closes #76

@mtinning mtinning requested a review from phil-brad January 22, 2019 16:33
Copy link
Member

@phil-brad phil-brad left a comment

Choose a reason for hiding this comment

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

I think it would be a good idea to add top-level run and inspect functions so that a user can simply do spawn.run(...), which mirrors the the CLI functionality. As a user I would expect to be able to do this. I think with your design of the interface classes, these top-level function can just build and call the appropriate interface class

@phil-brad
Copy link
Member

Following code errors for me inside spawn_config() (there should be a test on this I think):

import json
import spawn
with open('example_data/example_spec.json') as fp:
    spec_dict = json.load(fp)
spawn.inspect(spec_dict)

Also as a user, I would expect to be able to do spawn.inspect(spec_dict, outfile='spawn.json', format='json') as this parallels the CLI implementation. Also would be nice to pass in config parameters as arguments as this is more natural. I don't think it's necessary to be be able to take an argument config of type ConfigurationBase as this would be more in the realms of advanced users who could use the LocalInterface class. We could go for a **kwargs approach so that signature would be:

def inspect(spec_dict, outfile=None, format='txt', **kwargs)

@mtinning
Copy link
Member Author

Also as a user, I would expect to be able to do spawn.inspect(spec_dict, outfile='spawn.json', format='json') as this parallels the CLI implementation

I don't think this is the right way to go as it muddies the interface significantly. However I'd be happy with an additional method, write_inspection which took the inspection as an argument. So the interface would be something like:

spawn.write_inspection(spawn.inspect(spec_dict), outfile='spawn.json', format='json'))

@mtinning
Copy link
Member Author

Also, the definition is flexible, allowing config to be a dict

@phil-brad phil-brad merged commit 986e760 into master Feb 4, 2019
@phil-brad phil-brad deleted the spawn-interface branch February 4, 2019 22:58
@phil-brad
Copy link
Member

OK - I've merged this!

On the inspect function yes I agree actually, it should always return a dict (obviously a CLI cannot return a dict so this is not valid there) and should be abstracted from the write_inspection

However, I do still feel that there should be a top-level function where config parameters are default arguments. I saw that it could be a dict, but having it as a dict obfuscates the options a little because the user's IDE won't hint and the docstrings for that function can't tell them the options. It means they have to look into the config part of the docs which may not be immediately obvious to them. It's not about code design - which is clearly good, it's about offering a top-level wrapper function that delivers maximum usability. If you look at any major python package then it generally has top-level functions with a long list of default arguments (e.g. json.load, pandas.read_csv). These don't really reflect the underlying design but offer an incredibly easy-to-use interface for the user.

Alternatively, I could implement such top-level functions in spawn-wind but leave them out of the core spawn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants