Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing API calls: manage hosted repositories, get integrations #48

Merged
merged 16 commits into from
Jun 30, 2015

Conversation

cojoj
Copy link
Contributor

@cojoj cojoj commented Jun 28, 2015

This is the first commit for the series of Let's support official API. This is a PR for #40 and it'll take a while to complete, especially that some majore changes are being made in #44.

Fell free to review, comment and suggest changes 👀

- Optional retrieved integration.
- Optional operation error.
*/
public func retrieveIntegration(integrationId: String, completion: (integration: Integration?, error: NSError?) -> ()) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not insistent on keeping it this way, but so far I've tried to name most of these API calls with words starting with get, post etc, to sort of mirror the HTTP verb convention. There are exceptions, like createBot (which could just be postBot), but I think, for the sake of users' autocompletion, that we should keep the first words as predictable as possible. Here I'd name it just getIntegrations. Whadaya think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

- parameter integrations: Optional array of integrations.
- parameter error: Optional error.
*/
public func getIntegrations(completion: (integrations: [Integration]?, error: NSError?) -> ()) {
Copy link
Member

Choose a reason for hiding this comment

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

Btw, you can also use filters for integrations, see /Applications/Xcode-beta.app/Contents/Developer/usr/share/xcs/xcsd/routes/routes_integration.js. Not that you have to add it now, just FYI, probably something to add at some point in the future 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WOW, this is pretty cool! I'll add it for sure but I guess after #47 is complete so I don't get lost 😆

Copy link
Member

Choose a reason for hiding this comment

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

Haha yeah that makes sense. Let's split them first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@czechboy0 while working on #47 I've noticed thath your method: getBotIntegrations() use filters. Do you have any clue where I can find available keys for this :filter? query? While Charlesing around I've noticed only one:

  • last=

So I went to integrationFilterClass.js but guess what - can't find anything... 😕 Give me some hints as I want to include those to header docs.

Copy link
Member

Choose a reason for hiding this comment

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

See integrationSearchClass.js starting from line 124. Filters like from, last, number etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you‼️

I hate JS... 😕 Can't find anything in this mess! Hope Swift will replace it soon 😆

@cojoj
Copy link
Contributor Author

cojoj commented Jun 28, 2015

@czechboy0 @esttorhe please review Repository struct. I wonder if it needs graceful way of breaking or something or can it stay the way it is now? The problem may occur when we pass other value than Int to httpAccessType or sshAccessType but I guess it'll never happen...

@@ -106,6 +106,9 @@
3AA922BA1B3F4666005A0F73 /* DVR.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 3AA922AC1B3F4630005A0F73 /* DVR.framework */; };
3ABE68661B3AC3C500FA0A61 /* DeviceSpecification.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3ABE68651B3AC3C500FA0A61 /* DeviceSpecification.swift */; };
3ABE68671B3AC3C500FA0A61 /* DeviceSpecification.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3ABE68651B3AC3C500FA0A61 /* DeviceSpecification.swift */; };
7045917F1B4074CC00BA226C /* Repository.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7045917E1B4074CC00BA226C /* Repository.swift */; };
704591861B40945700BA226C /* RepositoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 704591851B40945700BA226C /* RepositoryTests.swift */; };
704591871B40945E00BA226C /* RepositoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 704591851B40945700BA226C /* RepositoryTests.swift */; };
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why it's duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a strange thing... Probably because of some kind of bug. I'll handle this 😉

Also, deleted duplication from .pbxproj file
@czechboy0
Copy link
Member

👍

@cojoj
Copy link
Contributor Author

cojoj commented Jun 29, 2015

getRepositories() and createRepository() are available, working and with header documentation. Also 100% unit tests coverage for Repository class 😉

Please review this and add your 2 cents 💵 (@esttorhe I'm talking to you 😆)

@cojoj
Copy link
Contributor Author

cojoj commented Jun 29, 2015

@czechboy0 I've changed the code to your suggestions, so feel free to review it one more time 👀

If you decide that what I've written in this PR is good enough to be merged please do so. I'd rather create new PR for other functionality not to confuse anyone.


public let name: String
public var httpAccess: HTTPAccessType = HTTPAccessType.None
public var sshAccess: SSHAccessType = SSHAccessType.SelectedReadWrite
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure SSHAccessType.SelectedReadWrite should be the default? I think you need to pass an array of identifiers for the people that can actually read and write (those "selected"). Doesn't this default, together with empty arrays of readAccessExternalIds and writeAccessExternalIds basically mean that nobody can read and write?

If so, I'd suggest making LoggedInReadWrite the default, because that's the only option not requiring a list of identifiers to make sense. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SSHAccessType.SelectedReadWrite is default and yes that means that nobody can access this repository if you pass empty arrays - strange but that's what I get in the response from XCS so I even double checked that 😜

Yeah, we can make LoggedInReadWrite as a default but I'll leave a comment if anyone will ever look for why we're not strict about this with XCS.

@czechboy0 czechboy0 changed the title [WIP] Add missing API calls Add missing API calls Jun 29, 2015
@czechboy0 czechboy0 changed the title Add missing API calls Add missing API calls: manage hosted repositories, get integrations Jun 29, 2015
@czechboy0
Copy link
Member

Great stuff, I just added a couple minor comments. Once you fix those, I'll happily merge this! Just one more thing - feel free to also change the readme to account for the API calls you just added! We want to be proud of how much we support! 😊

@cojoj cojoj mentioned this pull request Jun 30, 2015
@czechboy0
Copy link
Member

Amazing work. Thank you @cojoj! 👍

czechboy0 pushed a commit that referenced this pull request Jun 30, 2015
Add missing API calls: manage hosted repositories, get integrations
@czechboy0 czechboy0 merged commit b876c53 into buildasaurs:swift-2 Jun 30, 2015
@cojoj cojoj deleted the missing-api branch June 30, 2015 07:46
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