Remove RESTCommand and Result

[pranjalsatija]

As discussed in #3, RESTCommand and Result have been removed and replaced with a more powerful version of API.Endpoint. In addition, the remaining code has been refactored slightly to make it more organized.

[flovilmart]

Seems that the CI is not happy. Otherwise 1 small nit and looking great!

[flovilmart]

Can we inverse those and add back sema.signal()?

[pranjalsatija]

Just to clarify, it should read:

block({
    value = $0
    error = $1
    sema.signal()
})

Correct?

[flovilmart]

Yep!

[pranjalsatija]

@flovilmart Got it.

[flovilmart]

Also have a look about the linting installing swift-lint I believe tagr’s What breaks the CI

[pranjalsatija]

@flovilmart I actually have SwiftLint installed and running on my computer. I just tested it and it's definitely working.

[pranjalsatija]

Is it possible that Travis is misconfigured? It looks like its language is set to Objective C, but I'm not sure because I haven't worked with Travis a lot before:

{
  "osx_image": "xcode9",
  "language": "objective-c",
  "before_script": "brew update && brew upgrade swiftlint",
  "stage": "Build",
  "env": "TARGET=\"ParseSwift (macOS)\"",
  "script": "set -o pipefail && xcodebuild -target \"$TARGET\" | xcpretty",
  "jobs": {
    "include": [
      {
        "stage": "package managers",
        "env": "Swift Pacakge Manager",
        "script": "swift build"
      },
      {
        "stage": "package managers",
        "env": "Cocoapods",
        "script": "pod lib lint"
      },
      {
        "stage": "package managers",
        "env": "Carthage",
        "script": "carthage build --no-skip-current"
      }
    ]
  },
  "os": "osx"
}

It has a bunch of pretty weird compiler errors; it seems like it can't find a bunch of the types that are defined in the project.

[flovilmart]

Thar’s How Travis is supposed to be configured. Did you add all the files to all targets?

[pranjalsatija]

@flovilmart I just did.

[flovilmart]

Thanks!

[pranjalsatija]

@flovilmart I don't know why it's still failing. The error just says that the Run Script phase failed:

[pranjalsatija]

@flovilmart Any idea why it's still failing? Now it seems like it's not able to properly execute the script for SwiftLint.

iOS:

The following build commands failed:
	PhaseScriptExecution SwiftLint build/ParseSwift.build/Release-iphoneos/ParseSwift\ (iOS).build/Script-4A6511551F49D544005237DF.sh
(1 failure)

macOS:

The following build commands failed:
	PhaseScriptExecution SwiftLint build/ParseSwift.build/Release/ParseSwift\ (macOS).build/Script-4A6511541F49D53C005237DF.sh
(1 failure)

I looked at the build folder on my Mac, and I found Script-4A6511541F49D53C005237DF.sh. It just checks to make sure that SwiftLint is installed and echoes a warning if it's not:

#!/bin/sh
if which swiftlint >/dev/null; then
swiftlint
else
echo "warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint"
fi

One thing that's notable is that the path to the script that's shown in the log output is different than the path on my computer, but I'm 99% sure that's just because of the fact that Travis is building it differently than my computer does.

Do you have any ideas for what to do or try?

[flovilmart]

Likely a failure of SwiftLint. Can you comment it out and check if it’s all good?

…

On Sep 18, 2017, 14:28 -0400, Pranjal Satija ***@***.***>, wrote: @flovilmart Any idea why it's still failing? Now it seems like it's not able to properly execute the script for SwiftLint. iOS: The following build commands failed: PhaseScriptExecution SwiftLint build/ParseSwift.build/Release-iphoneos/ParseSwift\ (iOS).build/Script-4A6511551F49D544005237DF.sh (1 failure) macOS: The following build commands failed: PhaseScriptExecution SwiftLint build/ParseSwift.build/Release/ParseSwift\ (macOS).build/Script-4A6511541F49D53C005237DF.sh (1 failure) I looked at the build folder on my Mac, and I found Script-4A6511541F49D53C005237DF.sh. It just checks to make sure that SwiftLint is installed and echoes a warning if it's not: #!/bin/sh if which swiftlint >/dev/null; then swiftlint else echo "warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint" fi One thing that's notable is that the path to the script that's shown in the log output is different than the path on my computer, but I'm 99% sure that's just because of the fact that Travis is building it differently than my computer does. Do you have any ideas for what to do or try? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[pranjalsatija]

@flovilmart What do you mean? Just disable SwiftLint entirely for now?

[flovilmart]

Just to check if it's the culprit or not

[pranjalsatija]

@flovilmart My bad, I only disabled SwiftLint for iOS. The iOS build succeeded and the new commit has it disabled for macOS.

[flovilmart]

That means the issue is in the linting, so there’s something there that doesn’t sound right

[pranjalsatija]

@flovilmart I don't know what else could be causing it. Is there any way to see more detailed logs about why the script is failing to run?

[flovilmart]

The script was all working fine, I believe the changes make it trigger a failure. I’ll get your branch and have a look. The | xcpretty is swallowing the issues from this build step. I wish to keep the linting on as it gets harder to fix later. I’ll keep you posted on my findings.

[pranjalsatija]

I totally agree about keeping the linting on. I made the mistake of adding SwiftLint to a project that was already full of code and it’s a ton of work to fix all the little things it points out. Let me know if you need anything else from me!

[flovilmart]

You’ve already done plenty, so i’ll Check out if I find anything obvious, otherwise, in the .travis.yaml, we can remove the | xcpretty to get the full build logs

[flovilmart]

With swift lint back on obviously :p

[flovilmart]

@pranjalsatija
Thanks for holding in there.

• The issue with swift lint is identified, your version was most likely outdated, and it complained about a silence that was without side effect (49d76a1)

I've had a look at the PR, why did you remove the RESTBatchCommand, that's a feature that was there, and we can't move forward if for a particular refactor, we remove features.

Removing the Result type is quite nice.
RESTCOmmand should still be the bridge somehow between the Objects and the API, this way we can pack multiple commands into a single one. Also this helps with testability as every single save call should at one point issue a RESTCommand-ish before sending to the API.

[pranjalsatija]

@flovilmart my bad. I removed it temporarily to silence an error while I was working on updating the rest of the codebase to not use Result. I'll fix that.

What do you think the best way to incorporate RESTCommand or something similar is? We could build a RESTBridge or something that takes care of serialization and stuff. Would that work?

[flovilmart]

I'm not sure now, let me check and come back :)

[pranjalsatija]

Sure! Just let me know what you think is best.

[flovilmart]

but we need the RESTCommands so we can issue the batch command.

[flovilmart]

Maybe API is needed to be merged into REST instead of the opposite :)

[pranjalsatija]

I was actually planning on implementing batch functionality with an array of callbacks, or a single callback with an array of tuples.

[flovilmart]

The batch is a proper endpoint, it's not an array of callbacks.

[pranjalsatija]

Right. But do we need to bring back the entirety of RESTCommand to create that type of request? I'm not too familiar with Parse Server's batch API, but I'm pretty sure we need to make JSON like this:

[
  {
    "method": "POST",
    "path": "/1/classes/GameScore",
    "body": {
      "score": 1337,
      "playerName": "Sean Plott"
    }
  },
  {
    "method": "POST",
    "path": "/1/classes/GameScore",
    "body": {
      "score": 1338,
      "playerName": "ZeroCool"
    }
  }
]

But why do we need RESTCommand to do so? Can't we just build that JSON? We can already get the path through API.Endpoint, and method is constantly "POST". The only thing we really need to do is create the body dictionary, which is easy enough with the encoders that are already available.

What I meant when I mentioned an array of callbacks or a single callback with an array of tuples was my proposed external interface for calling the saveAll function:

[myObject1, myObject2, myObject3].saveAll {(objects, errors) in
  for (object, error) in zip(objects, errors) {
    //error handling
  }
}

[flovilmart]

Because the rest command abstracts nicely a body, an endpoint, and a meThod

[pranjalsatija]

So should we go back to how it was before? And merge API into RESTCommand? I'm still not sold on bringing it back. We could also do something like define an extension on ObjectType:

extension ObjectType {
  func makeBatchRequestDictionary() throws -> [String : Any] {
    return [
      "method" : API.Method.post.rawValue,
      "path" : API.Endpoint.objects(className: Self.className),
      "body" : try getJSONEncoder().encode(body)
    ]
  }
}

And then loop over an array of objects.

[flovilmart]

Also, the commands in the batch can be mixed as it’s supported by the server yet undocumented, so theoretically, you could have POST, PUT etc...

[pranjalsatija]

That's also solvable. We could have a parameter for makeRequestBatchDictionary():

extension ObjectType {
  func makeBatchRequestDictionary(method: API.Method) throws -> [String : Any] {
    return [
      "method" : method.rawValue,
      "path" : API.Endpoint.objects(className: Self.className),
      "body" : try getJSONEncoder().encode(body)
    ]
  }
}

I can undo the changes if you want, but I wasn't a fan of the RESTCommand concept in the first place, which is why I'd want to avoid it if we could. Plus, I really like the system that's in place now, and I feel like adding RESTCommand for just this one thing could introduce a lot of overhead for tasks that are currently accomplished pretty easily with standard callbacks.

[flovilmart]

The whole point of the rest command was to avoid having those dangling dictionaries, i’ll Work out something.

[pranjalsatija]

@flovilmart Let me know if I can contribute anything. Worst comes to worst, we can just roll back the last few commits and merge API with the REST stuff instead of the other way around.

[flovilmart]

I'm experimenting now with:

1. moving api execution (which is static anyway) into RESTCommand
2. making all calls synchronous, this will yield the benefit of letting the consumer choose it's async representation, make tests easier
3. probably introduce an executor, that would be replaceable

On an other hand, you seemed to be interested into the Current User/ Installation work, which need some porting from the iOS SDK, perhaps installation would be the 1st to do, as every single client call should have the installationId provided (and that will give basic storage etc..)

What do you think?

Sorry to have led you on a sub-par track, but things are getting better I guess. It's tough to make some architectural calls early on.

[pranjalsatija]

It's no problem! Honestly, I don't mind it. I get that this is still really early in its lifecycle so changes like this are to be expected. I thought about another approach, and that could be a RequestBuilder object or something, so we could do something like this:

do {
  let object: MyObjectType = try RequestBuilder.makeRequest(endpoint: .someEndpoint, body: myObject) {(error, response) in
     //do some kind of transformation on error and response to get an object
  }
} catch {
  //object is nil
}

Which is pretty similar to the way RESTCommand was, sans result.

As for User and Installation, I'm interested in working on them, yes. I'm interested in implementing all kinds of things in this SDK, just because I want to use it in my own app. I was waiting to get this PR settled and merged before I started working on that stuff more, but I can get to it if you think that should come first.

[flovilmart]

@pranjalsatija I've open a PR with what I had in mind after a few hours of tinkering around. I believe this is nicer now. Perhaps the API.Command.execute can go away, and we can provide an executor and a custom API for tests so we don't rely on parse-server (which would make it super nice)