search

keenlabs / keen-sdk-net

A .NET SDK for the Keen IO API
MIT License
37 stars 24 forks source link

Create Access Key #129

Open IshamMohamed opened 6 years ago

IshamMohamed commented 6 years ago

This is the pull request for the "Create Access Key"

masojus commented 6 years ago

Thanks, I'll continue review here.

masojus commented 6 years ago

I'm not sure why TravisCI succeeded and AppVeyor failed, but are your tests hitting the real server?

IshamMohamed commented 6 years ago

Strange, its coming from the last commit - I didn't see a problem in it.

masojus commented 6 years ago

The tests don't seem to be mocked, so if it was succeeding before it was because you hadn't switched the keys.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.8%) to 56.532% when pulling 43ea6214a06e43b0386d23694f06436e97460797 on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.2%) to 57.117% when pulling 0e2cd7a1b139fd3d917074d7d73027bfe1af2c0b on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 57.177% when pulling a781c51a9027dd3882e1446fc2d785c8ef946f4e on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 57.207% when pulling d2db29210d54dec6461e49b61d159abebb31b65d on IshamMohamed:master into b820c2ec8315e11a344ce4f239dc2709dd042240 on keenlabs:master.

IshamMohamed commented 6 years ago

Agreed to all of your changes.

masojus commented 6 years ago

@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?

IshamMohamed commented 6 years ago

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?

masojus commented 6 years ago

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.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.4%) to 69.682% when pulling 7abce954f7702ef0b05d8ca08b6bdf8264022dcb on IshamMohamed:master into 676e52fcd966db578103f74fa51b525c757df25e on keenlabs:master.

IshamMohamed commented 6 years ago

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.

masojus commented 6 years ago

Ok, if you push some more changes I should be able to review over the weekend. Thanks!

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.6%) to 69.476% when pulling b60d1b1b9889021bfe7039cd506fa51f1c53c6a5 on IshamMohamed:master into 676e52fcd966db578103f74fa51b525c757df25e on keenlabs:master.

masojus commented 6 years ago

@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.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.2%) to 72.512% when pulling a7e9a21e7845cd7a745e714df4739b251a9459eb on IshamMohamed:master into 536b56b440d0b91ee53bf067c856c98ff37e2946 on keenlabs:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.2%) to 72.512% when pulling b9fbe6fffb6ace40d7a56afa26f1f5762936860a on IshamMohamed:master into 536b56b440d0b91ee53bf067c856c98ff37e2946 on keenlabs:master.

IshamMohamed commented 6 years ago

@masojus can you please let me know, what has to be done to improve the code coverage?

masojus commented 6 years ago

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.

masojus commented 6 years ago

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.

masojus commented 6 years ago

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.

masojus commented 6 years ago

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.