Closed joguSD closed 8 years ago
Overall I like it. Bear with my pedantry however.
To start, could you please add full doc strings for every class and public method? Example.
I'd also really like to get loading the JSON started off right, in much the same way that we currently load data files in botocore and boto3. To help you get started, here's where boto3 sets up its loader: https://github.com/boto/boto3/blob/develop/boto3/session.py#L109. You'll need to create a data
folder here and set it up much like that. You can see how the loader is used a bit further down, but here's an example:
First, create a file called "wizards-1.json" in "~/.aws/models/myservice/2016-01-01/" with sample wizard contents.
import botocore.session
session = botocore.session.get_session()
loader = session.get_component('data_loader')
services = loader.list_available_services(type_name='wizards-1')
# service = = ['myservice']
model = loader.load_service_model(services[0], 'wizards-1')
Now what you have is an OrderedDict which is that json file. The advantage of doing it this way is that you now support all locations and future locations where customers or developers can put models.
Alright, I'm liking this a lot more. Now we need to get some more tests in. For reference, here's the coverage report:
============================= test session starts ==============================
platform darwin -- Python 3.5.2, pytest-2.8.2, py-1.4.31, pluggy-0.3.1
rootdir: , inifile:
plugins: cov-2.2.0
collected 8 items
tests/unit/test_wizard.py ........
--------------- coverage: platform darwin, python 3.5.2-final-0 ----------------
Name Stmts Miss Cover Missing
--------------------------------------------------
awsshell/wizard.py 115 30 74% 36-38, 53, 100-109, 157-169, 177, 180-181, 192-197, 212
=========================== 8 passed in 0.36 seconds ===========================
Of that, I think 192-197 can be skipped since those bits are placeholders.
Test coverage now includes all code except for the placeholder interaction step.
============================= test session starts =============================
platform darwin -- Python 3.5.1, pytest-2.8.2, py-1.4.31, pluggy-0.3.1
rootdir: , inifile:
plugins: cov-2.2.0
collected 14 items
tests/unit/test_wizard.py ..............
--------------- coverage: platform darwin, python 3.5.1-final-0 ---------------
Name Stmts Miss Cover Missing
--------------------------------------------------
awsshell/wizard.py 117 6 95% 203-208
========================== 14 passed in 0.37 seconds ==========================
Awesome! One last task before we can merge: let's squash up some of those commits.
Squashed down to 3 commits. Should I have been more aggressive or is 3 okay?
3 is fine. Most PRs should be scoped to one commit, but this one is necessarily larger.
___________________________________________________________________
|| * * (( * ||
|| * * * ~ ||
|| ___. * * ||
|| * ___.\__|.__. * ||
|| \__|. .| \_|. ||
|| . X|___|___| . * ||
|| .__/_||____ ||__. * /\ ||
|| * . |/|____ |_\|_ |/ _ / \ ||
|| \ _/ |_X__\|_ |\||~,~{ / \ ||
|| \/\ |/| |_ |/:|`X'{ _ _/ \__||
|| \ \/ |___ |_\|_.|~~~ / . .. . ..||
|| _|X/\ |___\|_ :| |_. - .......... . .||
|| | __\_:____ | ||o-| ___/........ . . .. ..||
|| |/_-|-_|__ \|_ |/--| ____/ . . .. . . .. ... . ||
|| ........:| -|- o-o\_:_\|o-/:....../...........................||
|| ._._._._._\=\====o==o==o=/:.._._._._._._._._._._._._._._._._._||
|| _._._._._._\_\ ._._._._.:._._._._._._._._._._._._._.P_._._._._||
|| ._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._||
||---------------------------------------------------------------||
What has been done and has tests:
Additional functionality
Beyond the backlog features above, some additional functionality has been given (basic) implementations to give some context for the above features.
These do not have tests as they require a functioning interaction step (at least mocking them).Edit: After review all functionality below has unit tests as well.