List all commits for integration

[cojoj]

Working now on List all commits for integration models.

[czechboy0]

Looking good so far!

[cojoj]

I wonder what status is in:

"XCSCommitCommitChangeFilePaths": [{
                    "status": 4,
                    "filePath": "XcodeServerSDK.podspec"
                }],

[czechboy0]

Changed, added or deleted? Maybe?

[czechboy0]

And moved?

[cojoj]

Yeah, you're right...

So far I got this:

• 1 - Added
• 2 - Deleted
• 4 - Changed/Modified
• 8192 - Moved

[cojoj]

Do you think that this is documented somewhere? MongoDB doesn't hold types but from reverse engineering you can see that this will be be based on bit modifications.

For now I think we can create enum for these 4 values and default case which will hold other values. If more statuses will recognized we can add them to enum.

[czechboy0]

:+1:

[czechboy0]

And btw I checked everywhere and couldn't find where all the file change statuses are defined.

[cojoj]

I guess it must be hardcoded somewhere so we won't get access to this... 😢

[czechboy0]

I don't think it's intentional, tbh. Anyway, put it in an enum with Int raw value.

[cojoj]

So far, I got this

public enum FileStatus: Int {
    case Added = 1
    case Deleted = 2
    case Modified = 4
    case Moved = 8192
}

I guess it isn't possible to call something like:

FileStatus(rawValue: 8)

So we should make it Optional because it's be a failable initializer?

[cojoj]

Came with this. What do you think @czechboy0?

public enum FileStatus: Int {
    case Added = 1
    case Deleted = 2
    case Modified = 4
    case Moved = 8192
    case Other

    public init?(rawValue: Int) {
        switch rawValue {
        case 1:
            self = .Added
        case 2:
            self = .Deleted
        case 4:
            self = .Modified
        case 8192:
            self = .Moved
        default:
            self = .Other
        }
    }
}

One thing that hurts me is the fact that we can't store value for .Other and if rawValue computed property will be called I can't return the proper value for this one... 😢

[czechboy0]

Don't enums already have failable initializers generated by default?

[czechboy0]

So in the Commit you'd do let fileStatus = FileStatus(rawValue: parsed) ?? .Other

[cojoj]

Yes, they have but I was hoping I can somehow assign other value to .Other but I guess it's impossible...

[czechboy0]

Yeah I'm afraid enums don't have a default value, which is basically what you're looking for. Radar?

[cojoj]

It's a bit controversial... Should enums have default values? But on the other hand, they already store rawValues so where's the problem to assign .Other value?

[czechboy0]

Well, they have a failable initializer. It'd be nice to have the enum, instead of potentially returning nil, when the rawValue doesn't match a case, return one of the predefined cases (the default case). Something like Hash default values in Ruby, which you can tell to return a specific value when you're trying to pull a value for an unknown key.

It would be nice to allow the enum to have a non-failable initailizer, because it'd always return one of the cases.

[cojoj]

Exactly but that's probably why failable initializers are default ones in enums...

[cojoj]

@czechboy0 we have an answer straight from  about our little enum thing:

public enum FileStatus: RawRepresentable {
  case Added, Deleted, Modified, Moved, Other(Int)

  init(rawValue: Int) {
    switch rawValue {
    case 1: self = Added
    case 2: self = Deleted
    case 4: self = Modified
    case 8192: self = Moved
    default: self = Other(Int)
  }

  var rawValue: Int {
    switch self {
    case Added: return 1
    case Deleted: return 2
    case Modified: return 4
    case Moved: return 8192
    case Other(let x): return x
    }
  }
}

[czechboy0]

Hmm interesting, but is all this code worth it? I still don't hate let fileStatus = FileStatus(rawValue: parsed) ?? .Other as much I'd hate to have this much code to achieve the same thing :(

And most importantly, the values are duplicated here, so if they ever change/we add a new one, you have to change it in two places.

[cojoj]

The thing I keep in mind when thinking about this is we only know 4 values for status. I want to keep somehow value of statuses which aren't known yet... ← This is my personal opinion.

On the other hand - this will be an official API call so there should be a documentation for this soon (they've promised beta 3 or 4 but still not here...). If we get ths documentation we can easily add missing cases and live happily after all 😉

I hate making decisions... So, it's up to you 😜

[czechboy0]

Yeah I get your point. But IMHO, if we do see .Other state somewhere, we can always debug and take a look at the actual value. Since we don't need the value anywhere else (at the moment), I would only see this as a small help during debugging. Which, as you said, hopefully should not be required if Apple gives us official docs on this.

I'd go with the simpler solution for now.

[cojoj]

Fair enough! 👍

[czechboy0]

Can you please merge czechboy0/swift-2 into this branch? I made changes for Xcode7b4 compatibility.

[cojoj]

Done with merging...

[cojoj]

results contain array... In our case it'll be of type [IntegrationCommits]. Do you think thath it's possible there'll be more than one element in this results array? If yes then I think name of this class should be changed... If no, we should return only one object of type IntegrationCommits.

[cojoj]

This is my implementation of init - just code, without any proof... We'll have to record a cassete and write request method for this. I still think that the name commits in this JSON is confusing, so I just flatMapped this shit... 💩

[buildasaur]

Result of Integration 2

Duration: 1 minute and 4 seconds Result: Perfect build! All 36 tests passed. :+1: Test Coverage: 48%.

[cojoj]

Do you really think it'd be logical to have a Dictionary instead of Array as commits property? Imagine yourself using this...

"I just want to see how many commits have this integration" "WTF, why it's a Dictionary of freaking Strings instead of [Commit]!?"

That's why I'm using here flatMap, so when I access commits object in JSON and get Dictionary of type [String: [Commit]] from it I'm taking allValues, so the array of arrays and flatten it so we'll have one dimensional array. Why I'm flatMapping? As we've discussed - we may have more RepositoryBlueprintIDs (which we don't know how to prove, yet).

[czechboy0]

I think a repository with git submodules will have multiple repository ids.

But yes, I think we should map the returned objects as closely as possible. The user can always do the flat map themselves, but right now we're taking away information about which repo generated those commits.

The purpose of this project is to map the JSON api into Swift objects. Plus we have a couple of utility functions. But we shouldn't be dictating what information the user doesn't need to see, if we have it. That's up to them to decide. So yes, I think the type should be preserved from the JSON, [String: [Commit]].

[cojoj]

From my POV, Apple screwed naming... Why commits? 😑

[czechboy0]

Yeah maybe. commits as in this is where you get commits. But they are keyed by repo. What else would you name it?

[cojoj]

Now we can't do anything... But when they post docs for this endpoints and we'll be assured that those are submodules, we can name it submodules...

[cojoj]

Got any pattern on how to work with conversion from NSDictionary → Dictionary?

Also, please review in free time this pull request as it's some kind of blocker for me 😓

[czechboy0]

1. I merged you PR in BuildaUtils, in the Podfile change the BuildaUtils version to 0.0.6.
2. Just do an optional cast from NSDictionary to your type of Dictionary. It will check the objects inside to make sure it contains the right objects. Is that what you mean?

[buildasaur]

Result of Integration 3

Duration: 44 seconds Result: Perfect build! All 37 tests passed. :+1: Test Coverage: 49%.

[cojoj]

Lately I changed my opinion and now I think we should really minimize the places where we force unwrap optionals.

Now, change your opinion and let's drop all those NSDictionaries 😍

---

I will try it later... Now it's pizza Friday!!! 🍕

[czechboy0]

How do you mean? Where do you think we shouldn't be using NSDictionarys?

[cojoj]

Call me hipster or whatever but I really hate mixing Swift with Objective-C... For me, this project is totally platform independant (we don't use any of UI stuff specific to OS) that's why I see a possibility to do it Swifty.

I know that parsing JSON without NSDictionary will be a PITA but. hey, it's Open Source, we have tame, we have possibilities and we're willing to learn new stuff, right?

Because of these easy casting to NSDictionary or NSArray we're fighting with Swift's static typing and (I think so) losing some security.

---

It's still your project and I'm only, a contributor so... I'll do it by following your leads but I only wanted to share my honest opinion 😉

[czechboy0]

In principle I agree, but JSON is a special case IMHO. Even if we have a native JSON object in Swift, it will still be dynamic when it comes to its contents, because it can contain any sorts of stuff. So we won't get any added security because the types are not known at compile time.

I agree it would be nice to get rid of all NS classes, but in practice I'm not sure I understand what you're proposing :)

[cojoj]

You probably won't exactly understand what I mean as this is only me talking, so don't bother 😉

I hate that we have this Objective-C technical debt in every code we write right now... 😭

[czechboy0]

Yeah I feel you, but it could be much worse, trust me. Plus, most of Apple's energy and time seems to be poured into Swift now. There is hope :wink:

[czechboy0]

Just a couple of comments I added - awesome stuff as usual, @cojoj! Thanks for all your work here :+1:

[czechboy0]

Let me know when this is ready, @cojoj! :wink:

[cojoj]

Probably tomorrow as I don't know if I'll have time today to do some coding... 😕

[buildasaur]

Result of Integration 4

Duration: 53 seconds Result: Perfect build! All 56 tests passed. :+1: Test Coverage: 54%.

[buildasaur]

Result of Integration 5

Duration: 48 seconds Result: Perfect build! All 56 tests passed. :+1: Test Coverage: 54%.

[cojoj]

Casting fixed (looks neat)! I'll now spend a while with writing some test cases and after that I think we'll be ready with this.

[buildasaur]

Result of Integration 6

Duration: 57 seconds Result: Perfect build! All 56 tests passed. :+1: Test Coverage: 54%.