-
Notifications
You must be signed in to change notification settings - Fork 30
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
Conversation
- Optional retrieved integration. | ||
- Optional operation error. | ||
*/ | ||
public func retrieveIntegration(integrationId: String, completion: (integration: Integration?, error: NSError?) -> ()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?) -> ()) { |
There was a problem hiding this comment.
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 😉
There was a problem hiding this comment.
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 😆
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😆
@czechboy0 @esttorhe please review |
@@ -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 */; }; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
👍 |
Please review this and add your 2 cents 💵 (@esttorhe I'm talking to you 😆) |
@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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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! 😊 |
Amazing work. Thank you @cojoj! 👍 |
Add missing API calls: manage hosted repositories, get integrations
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 👀