GitLabKit –> TanukiKit

[tiferrei]

This is the pull request for issue #8
( @pietbrauer Sorry about deleting the old one and opening this one, had to do some house cleaning on the branches.)

For anyone else, the idea of this pull request will be to basically extend TanukiKit to cover basically the entire GitLab API. We'll start by adding the GET requests, then the POST, then the DELETE and finally the PUT requests.

@pietbrauer , I was thinking of creating a standard structure for each file:

• No typical Xcode header on files.
• // MARK: Router the router segment.
• The file name must start with capital and if it as a:
  • GET, just use the name of what you're getting, for example: Builds.swift or Commits.swift.
  • POST, start with Send and then the name of what you're posting, for example: SendKeys.swift.
  • DELETE, start with Remove and then the name of what you're deleting, for example: RemoveKeys.swift.
  • PUT, start with Update and then the name of what you're updating, for example: UpdateKeys.swift.
• The main class for the file should be of style: api + name capitalised and on singular form + Class, for example: apiBuildClass, not apiBuildsClass, or for a POST, apiSendKeyClass.
• Xcode document all main functions.
• The main function name must be the file name camelised.
• All functions must have tests with the Nocilla framework.

(Open to suggestions)

[pietbrauer]

Hey Tiago, first of all thank you for your Pull Request!

There are a couple of things I would like you to clean up before I start to review:

• You changed the permissions of almost every file from 0644 to 0755 afaik this was unnecessary, please revert that
• file names are the resource name
• Class names begin with a capital letter
• Class names do not need to say that they have something to do with API or Class, the first one is implicit as this is a Swift API client for GitLab. The second one is just Swift convention, e.g. you don't see NSStringClass or AnyObjectClass as the user of the library already knows it is a class.
• There is a lot of noise going on in the .pbxproj and I don't know why this was necessary. The only change should be the added file
• Why was Repositories renamed to Projects? This is a breaking API change and I would have to release a new major version for this change, which could have been prevented. I would also like to have one change at a time and the change for this particular pull request is to add build support
• Tests are missing and you broke the other tests

There is also an outstanding architectural changes in #6, just a heads up but nothing to worry about yet.

[tiferrei]

Thanks for the heads up @pietbrauer:

1. File permissions make sense and will be fixed, did that by mistake!
2. Agreed, will update that on the initial message.
3. Also agreed, forgot by mistake, will be fixed on next commit.
4. Right, this one is a bit hard to explain, but the main thing is, I bet most people using this framework will reference these classes in other places, for example to save the data to an array of objects:

class User {
    var login: String = ""
    var bio: String? = nil
    var name: String = ""
    var email: String = ""
    var privateToken: String = ""
    var avatarURL: String? = nil
    var webURL: String = ""
    var websiteURL: String? = nil
    var skype: String? = nil
    var linkedin: String? = nil
    var twitter: String? = nil
    var isAdmin = false
    var projectsLimit = 0
    var canCreateProject = false
    var canCreateGroup = false

    func addUserData(someAPIUser: APIUserClass) {
        name = someAPIUser.name!
        login = someAPIUser.login!
        bio = someAPIUser.bio
        email = someAPIUser.email!
        privateToken = someAPIUser.privateToken!
        avatarURL = someAPIUser.avatarURL
        webURL = someAPIUser.webURL!
        websiteURL = someAPIUser.websiteURL
        skype = someAPIUser.skype
        linkedin = someAPIUser.linkedin
        twitter = someAPIUser.twitter
        isAdmin = someAPIUser.isAdmin!
        projectsLimit = someAPIUser.projectsLimit!
        canCreateGroup = someAPIUser.canCreateGroup!
        canCreateProject = someAPIUser.canCreateProject!
        listOfUserData.append(User())
    }
}

As you can see this is quite a popular scenario, and the user would prefer to be able to call their class User instead of something else just because TanukiKit already uses that name, it would also cause confusion later, I do agree though with the removing the Class part, but I just think we should have a prefix on the class name to indicate it is the TanukiKit one, please tell which way you prefer.
5. I also noticed the noise, although I haven't touched that file in any way! Will check that.
6. I do realise this would mean changing the functions on the programs, but it is extremely necessary! Why? Firstly, because on GitLab they are called Projects, not repositories, also the API clearly treats them as projects, (https://gitlab.com/api/v3/projects) but that's acceptable, what really is a concern is that on GitLab repos are actually the files managed by git itself, the bare propose of the project let's say, and that's the way gitlab references them, so later, when I added the repository request, it would cause a big bowl of spaghetti mess.
7. One change at the time makes complete sense, this will be the builds one.
8. Firstly, sh**, I didn't even touch the tests yet, don't know how I broke them, will take a look at that, secondly, haven't done the test for the builds yet because I realised that you only seem to have tests for the POST requests, like the keys, do you also want tests for the GETs?

Finally, I also usually work with Swift-Clean on Xcode, an amazing extension that works as a build phase that marks as warnings stuff like variables that start with a capital letter and shouldn't etc. are you interested in keeping this or should I only use it locally?

Tiago

PS: Was thinking of doing a CONTRIBUTING.md with these guides later, approved?

[tiferrei]

@pietbrauer, commit 11cc33e (Fixed points 1, 2 and 3. (pull request #10)), fixes the permissions, updated the class names to point 3 until I wait for you to evaluate my request on naming.

I don't think I actually broke the tests, firstly because I didn't change the code, I literally just fixed the indentation because it was a bit messed up. I did find however that travis seems to be failing because of a Could not find jwt-1.5.3 in any of the sources error. What that is I don't know, but it seems to me like a dependency is missing, doesn't seem connected to the code itself.

The noise on the project file is because of A) the name change to TanukiKit instead of GitLabKit, you seem to have forgotten to change it after you found out GitLabKit already existed and B) to link the Pods required to develop the framework (if you handle the dependencies in any other way please feel free to say how and I'll change it).

Tiago

[pietbrauer]

I use Carthage simply install using brew install carthage and then carthage bootstrap. This will download RequestKit and the tests should just run.

Regarding the error just cherry-pick c822abb into your branch.

I understand your point about the repositories/projects change, can you please do it in a separate PullRequest? And please do a separate PullRequest for the TanukiKit/GitlabKit change.

Regarding the class naming: Swift has modules and names have to be exclusive in a module, e.g. TanukiKit. If your app defines a class User what the swift compiler sees is MyApp.User (ModuleName.Class) and TanukiKit.User. Objective-C did not have this functionality and therefore you had to use class prefixes. This is not necessary with Swift anymore.

[tiferrei]

Yup got it, will start by removing cocoa pods and use carthage, then I will do a pull request with the TanukiKit change, then once that gets approved, I'll open another with the Projects change, then add the commits and runner first, as they are necessary later for the builds.

Got it, will use the normal class names then.

Tiago

[tiferrei]

Alright, @pietbrauer, this will be the GitLabKit --> TanukiKit pull request, figured out what was wrong with the tests and as you can see everything is working now, also applied already the changes from RequestKit 1.0

Tiago

[pietbrauer]

Hm, why exactly did you update to RequestKit 1.0? There is a pull request which was just waiting for this here to finish. Have a look at #11. That's multiple changes in one again. Also please don't change the file order. Please open a new pull request and change the name from GitLabKit to TanukiKit.

[tiferrei]

Got it, sorry, third time's the charm right?