keen-sdk-net
keen-sdk-net copied to clipboard
keenlabs
Create Access Key
I'm not sure why TravisCI succeeded and AppVeyor failed, but are your tests hitting the real server?
Strange, its coming from the last commit - I didn't see a problem in it.
The tests don't seem to be mocked, so if it was succeeding before it was because you hadn't switched the keys.
Coverage decreased (-0.8%) to 56.532% when pulling 43ea6214a06e43b0386d23694f06436e97460797 on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.
Coverage decreased (-0.2%) to 57.117% when pulling 0e2cd7a1b139fd3d917074d7d73027bfe1af2c0b on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.
Coverage decreased (-0.1%) to 57.177% when pulling a781c51a9027dd3882e1446fc2d785c8ef946f4e on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.
Coverage decreased (-0.1%) to 57.207% when pulling d2db29210d54dec6461e49b61d159abebb31b65d on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.
@IshamMohamed Are you actively working on this for the rest of the week? What's your ideal timeline for making these suggested revisions? Do you have plans to add the other Access Keys APIs to this, or to leave that for another PR?
Yes I will be. Will finish the suggested revisions before end of this week and start working on the other APIs - but I don't really know if I need another PR for it? do I?
Well we'll need to approve and merges changes to master. Speaking of, you'll need to merge from master to get the latest and deal with any potential merge conflicts.
Coverage decreased (-0.4%) to 69.682% when pulling 7abce954f7702ef0b05d8ca08b6bdf8264022dcb on IshamMohamed:master into 676e52fcd966db578103f74fa51b525c757df25e on keenlabs:master.
Reviewed all your changed and accepted them. Will work a bit on improving code coverage later today - and come back to some of your questions as well.
Ok, if you push some more changes I should be able to review over the weekend. Thanks!
Coverage decreased (-0.6%) to 69.476% when pulling b60d1b1b9889021bfe7039cd506fa51f1c53c6a5 on IshamMohamed:master into 676e52fcd966db578103f74fa51b525c757df25e on keenlabs:master.
@IshamMohamed Let me know when you have something ready for review again. I'd love to get AccessKey creation checked in so we can flesh out the rest of the functionality around AccessKeys.
Coverage decreased (-0.2%) to 72.512% when pulling a7e9a21e7845cd7a745e714df4739b251a9459eb on IshamMohamed:master into 536b56b440d0b91ee53bf067c856c98ff37e2946 on keenlabs:master.
Coverage decreased (-0.2%) to 72.512% when pulling b9fbe6fffb6ace40d7a56afa26f1f5762936860a on IshamMohamed:master into 536b56b440d0b91ee53bf067c856c98ff37e2946 on keenlabs:master.
@masojus can you please let me know, what has to be done to improve the code coverage?
A difference of -0.2% isn't a huge deal, but you could look at where code isn't covered and add tests to hit those lines. For example if you look here and here there are some lines that aren't hit, so you could write tests to exercise those setters/getters and hit those error cases.
Also, there are some huge changes in master now in preparation for releasing a 0.4.0 .nupkg, but tomorrow I plan on taking these changes and moving them into a branch off of master with all the new stuff and continuing the PR discussion over there...that way I can help deal with all these huge changes and we can get the CreateAccessKey() logic in at least.
Okay, I merged master and will push a branch...I'll probably create a new PR tomorrow. You missed several of the comments and changes I requested in this PR, so we'll have to go through and clean those things up. Expect to see those changes tomorrow.
Take a look at jm_CreateAccessKey_PR129 to see how things are going with porting this to the new project structure and starting some clean up. I'll make more progress tomorrow and ask for review.