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 Task.requestCustomJSONEncodable #1443

Merged
merged 7 commits into from Nov 29, 2017

Conversation

Projects
None yet
10 participants
@evgeny-sureev
Contributor

evgeny-sureev commented Nov 3, 2017

Fixes #1442

@MoyaBot

This comment has been minimized.

Show comment
Hide comment
@MoyaBot

MoyaBot Nov 3, 2017

SwiftLint found issues

Warnings

File Line Reason
TestHelpers.swift 55 Operators should be surrounded by a single whitespace when defining them.

Generated by 🚫 Danger

MoyaBot commented Nov 3, 2017

SwiftLint found issues

Warnings

File Line Reason
TestHelpers.swift 55 Operators should be surrounded by a single whitespace when defining them.

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 3, 2017

Codecov Report

Merging #1443 into development will increase coverage by 0.59%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1443      +/-   ##
===============================================
+ Coverage        87.82%   88.41%   +0.59%     
===============================================
  Files                5        5              
  Lines              156      164       +8     
===============================================
+ Hits               137      145       +8     
  Misses              19       19
Impacted Files Coverage Δ
Sources/ReactiveMoya/MoyaProvider+Reactive.swift 91.89% <0%> (+0.98%) ⬆️
Sources/RxMoya/MoyaProvider+Rx.swift 91.66% <0%> (+1.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e729b3a...d3de41f. Read the comment docs.

codecov-io commented Nov 3, 2017

Codecov Report

Merging #1443 into development will increase coverage by 0.59%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1443      +/-   ##
===============================================
+ Coverage        87.82%   88.41%   +0.59%     
===============================================
  Files                5        5              
  Lines              156      164       +8     
===============================================
+ Hits               137      145       +8     
  Misses              19       19
Impacted Files Coverage Δ
Sources/ReactiveMoya/MoyaProvider+Reactive.swift 91.89% <0%> (+0.98%) ⬆️
Sources/RxMoya/MoyaProvider+Rx.swift 91.66% <0%> (+1.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e729b3a...d3de41f. Read the comment docs.

@@ -175,6 +175,45 @@ final class EndpointSpec: QuickSpec {
}
}
context("when task is .requestCustomJSONEncodable") {

This comment has been minimized.

@SD10

SD10 Nov 3, 2017

Member

Maybe we can add test case for when encoding fails?

@SD10

SD10 Nov 3, 2017

Member

Maybe we can add test case for when encoding fails?

@SD10 SD10 changed the base branch from master to development Nov 3, 2017

@SD10 SD10 changed the base branch from development to master Nov 3, 2017

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 3, 2017

Member

Hey @evgeny-sureev, thanks for doing this 👍 I think this PR is better targeted at development because the addition of a new Task case is an API breaking change. However, I see that development needs to be updated to reflect master.

If you could also take care of some of the SwiftLint errors that would be great 👍 The first one is regarding complexity so I'm not sure how easy it is to resolve

Member

SD10 commented Nov 3, 2017

Hey @evgeny-sureev, thanks for doing this 👍 I think this PR is better targeted at development because the addition of a new Task case is an API breaking change. However, I see that development needs to be updated to reflect master.

If you could also take care of some of the SwiftLint errors that would be great 👍 The first one is regarding complexity so I'm not sure how easy it is to resolve

@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 4, 2017

Contributor

First SwiftLint error is Cyclomatic Complexity Violation, it is unavoidable in switches for enums with many cases.

Contributor

evgeny-sureev commented Nov 4, 2017

First SwiftLint error is Cyclomatic Complexity Violation, it is unavoidable in switches for enums with many cases.

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Nov 6, 2017

Member

@evgeny-sureev can associated values in an enum case be given defaults? If so, maybe it would be nice to add an encoder property to the requestJSONEncodable case instead of creating a new one. For people who don't want to change the default value, they need not change it

Member

AndrewSB commented Nov 6, 2017

@evgeny-sureev can associated values in an enum case be given defaults? If so, maybe it would be nice to add an encoder property to the requestJSONEncodable case instead of creating a new one. For people who don't want to change the default value, they need not change it

@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 6, 2017

Contributor

This would be best variant, but Swift doesn't support defaults for enum associated values yet.

Contributor

evgeny-sureev commented Nov 6, 2017

This would be best variant, but Swift doesn't support defaults for enum associated values yet.

@Istered

This comment has been minimized.

Show comment
Hide comment
@Istered

Istered Nov 8, 2017

You can use static functions to "mimic" cases with default values.
Something like this:

public enum Task {
   ....
    case requestCustomJSONEncodable(Encodable, encoder: JSONEncoder)

    public static func requestJSONEncodable(_ encodable: Encodable, encoder: JSONEncoder = JSONEncoder()) -> Task {
        return .requestCustomJSONEncodable(encodable, encoder: encoder)
    }
   ....
}

Or even skip the second parameter:

public static func requestJSONEncodable(_ encodable: Encodable) -> Task {
    return .requestCustomJSONEncodable(encodable, encoder: JSONEncoder())
}

Istered commented Nov 8, 2017

You can use static functions to "mimic" cases with default values.
Something like this:

public enum Task {
   ....
    case requestCustomJSONEncodable(Encodable, encoder: JSONEncoder)

    public static func requestJSONEncodable(_ encodable: Encodable, encoder: JSONEncoder = JSONEncoder()) -> Task {
        return .requestCustomJSONEncodable(encodable, encoder: encoder)
    }
   ....
}

Or even skip the second parameter:

public static func requestJSONEncodable(_ encodable: Encodable) -> Task {
    return .requestCustomJSONEncodable(encodable, encoder: JSONEncoder())
}
@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 11, 2017

Member

This is really smart @Istered! Doesn't compiler complain about it, though? 🤔

Member

sunshinejr commented Nov 11, 2017

This is really smart @Istered! Doesn't compiler complain about it, though? 🤔

@BasThomas

This comment has been minimized.

Show comment
Hide comment
@BasThomas

BasThomas Nov 11, 2017

Contributor

That should just work - we're actually doing something similar already here.

Contributor

BasThomas commented Nov 11, 2017

That should just work - we're actually doing something similar already here.

@Istered

This comment has been minimized.

Show comment
Hide comment
@Istered

Istered Nov 13, 2017

@sunshinejr It will work as long as functions and cases have different names.

Istered commented Nov 13, 2017

@sunshinejr It will work as long as functions and cases have different names.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 13, 2017

Member

Oh, so even though the parameter list is different, it still won't work right @Istered? Bummer. Then we probably can't make it pretty without duplicating cases, right?

Member

sunshinejr commented Nov 13, 2017

Oh, so even though the parameter list is different, it still won't work right @Istered? Bummer. Then we probably can't make it pretty without duplicating cases, right?

@Istered

This comment has been minimized.

Show comment
Hide comment
@Istered

Istered Nov 13, 2017

@sunshinejr Yes, it won't work even though parameter list is different, but still there's only one case you need to handle with switch.

So instead of this:

switch task {
    ....
     /// These are two different cases to handle Encodable in this PR
    case let .requestJSONEncodable(encodable):
     ///

    case let .requestCustomJSONEncodable(encodable, encoder):
     ///
    ....
}

You would write only this:

switch task {
    ....
     /// 
    case let .requestCustomJSONEncodable(encodable, encoder):
     ///
    ....
}

Istered commented Nov 13, 2017

@sunshinejr Yes, it won't work even though parameter list is different, but still there's only one case you need to handle with switch.

So instead of this:

switch task {
    ....
     /// These are two different cases to handle Encodable in this PR
    case let .requestJSONEncodable(encodable):
     ///

    case let .requestCustomJSONEncodable(encodable, encoder):
     ///
    ....
}

You would write only this:

switch task {
    ....
     /// 
    case let .requestCustomJSONEncodable(encodable, encoder):
     ///
    ....
}
@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 14, 2017

Contributor

@Istered done.

Contributor

evgeny-sureev commented Nov 14, 2017

@Istered done.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 14, 2017

Member

I'm not sure I agree with this approach. We're avoiding adding another enum case (technically replacing an existing one) by adding a static method.

I don't think there is anything to gain here, we're simply moving the definition from the case level to the method level and making the API less intuitive.

I also really dislike that the default name of this task is now requestCustomJSONEncodable

Member

SD10 commented Nov 14, 2017

I'm not sure I agree with this approach. We're avoiding adding another enum case (technically replacing an existing one) by adding a static method.

I don't think there is anything to gain here, we're simply moving the definition from the case level to the method level and making the API less intuitive.

I also really dislike that the default name of this task is now requestCustomJSONEncodable

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 15, 2017

Member

I agree completely with @SD10. This is why I don't see much advantages in that approach with our current situation.

Member

sunshinejr commented Nov 15, 2017

I agree completely with @SD10. This is why I don't see much advantages in that approach with our current situation.

@Istered

This comment has been minimized.

Show comment
Hide comment
@Istered

Istered Nov 15, 2017

@SD10 @sunshinejr
Maybe it's just me, but I don't see any problems with that since implicit member expressions work with static functions the same way as with enum cases.

What really confuses me are these two lines doing absolutely the same:

return .requestJSONEncodable(...)
return .requestCustomJSONEncodable(..., JSONEncoder())

But I agree with you, guys, it would be better to have only one case. However, the problem is that you can't specify default value for cases as it was mentioned above and also it will be a breaking change if you replace requestJSONEncodable(_) with requestJSONEncodable(_, _), right?

Istered commented Nov 15, 2017

@SD10 @sunshinejr
Maybe it's just me, but I don't see any problems with that since implicit member expressions work with static functions the same way as with enum cases.

What really confuses me are these two lines doing absolutely the same:

return .requestJSONEncodable(...)
return .requestCustomJSONEncodable(..., JSONEncoder())

But I agree with you, guys, it would be better to have only one case. However, the problem is that you can't specify default value for cases as it was mentioned above and also it will be a breaking change if you replace requestJSONEncodable(_) with requestJSONEncodable(_, _), right?

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 15, 2017

Member

@Istered I'm not worried about breaking the API, breaking changes are inevitable and this requires a very simple migration.

// These may "do" the same thing but the way they are defined have implications
return .requestJSONEncodable(...)
return .requestCustomJSONEncodable(..., JSONEncoder())

What bothers me is there is no value in defining requestJSONEncodable(Encodable, encoder: JSONEncoder) as a static method.

  • It's true that there will be 1 less enum case but the code footprint is no smaller, it actually takes more code to define a method.

  • It makes the API non-intuitive, Task.requestJSONEncodable looks like an enum case to the user but it is really a method that returns a Task.requestCustomJSONEncodable.

  • We no longer get the added benefit of exhaustiveness for having this defined as an enum case. You won't be able to differentiate between Task.requestCustomJSONEncodable and Task.requestJSONEncodable.

I think we should solve this problem 1 of two ways:

a) We make one case requestJSONEncodable(Encodable, encoder: JSONEncoder) and the user just has to use a boilerplate JSONEncoder() or create their own convenience property.

b) We have two cases: requestJSONEncodable(Encodable) and requestCustomJSONEncodable(Encodable, encoder: JSONEncoder)

Member

SD10 commented Nov 15, 2017

@Istered I'm not worried about breaking the API, breaking changes are inevitable and this requires a very simple migration.

// These may "do" the same thing but the way they are defined have implications
return .requestJSONEncodable(...)
return .requestCustomJSONEncodable(..., JSONEncoder())

What bothers me is there is no value in defining requestJSONEncodable(Encodable, encoder: JSONEncoder) as a static method.

  • It's true that there will be 1 less enum case but the code footprint is no smaller, it actually takes more code to define a method.

  • It makes the API non-intuitive, Task.requestJSONEncodable looks like an enum case to the user but it is really a method that returns a Task.requestCustomJSONEncodable.

  • We no longer get the added benefit of exhaustiveness for having this defined as an enum case. You won't be able to differentiate between Task.requestCustomJSONEncodable and Task.requestJSONEncodable.

I think we should solve this problem 1 of two ways:

a) We make one case requestJSONEncodable(Encodable, encoder: JSONEncoder) and the user just has to use a boilerplate JSONEncoder() or create their own convenience property.

b) We have two cases: requestJSONEncodable(Encodable) and requestCustomJSONEncodable(Encodable, encoder: JSONEncoder)

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 17, 2017

Member

@evgeny-sureev Can we revert the last commit and move forward on merging this? That would be in line with option (B) I mentioned in my previous comment. Unless any @Moya/contributors disagree with me of course

Member

SD10 commented Nov 17, 2017

@evgeny-sureev Can we revert the last commit and move forward on merging this? That would be in line with option (B) I mentioned in my previous comment. Unless any @Moya/contributors disagree with me of course

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Nov 17, 2017

Member

@SD10: tbh I prefer (a), it seems more complete a solution

Member

AndrewSB commented Nov 17, 2017

@SD10: tbh I prefer (a), it seems more complete a solution

@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 17, 2017

Contributor

I think I was too fast with last commit. Feel free to revert it.

Contributor

evgeny-sureev commented Nov 17, 2017

I think I was too fast with last commit. Feel free to revert it.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 20, 2017

Member

We have already a method that requires only to pass Encodable and if you don't worry about your JSONEncoder (which is probably the case most of the times), it is tedious to pass an empty instance to the method all the time (we have it already e.g. request with/without parameters/encoding etc.). Thus, I'm more keen to just add another case to our Task type.

Thoughts @evgeny-sureev @SD10 @AndrewSB?

Member

sunshinejr commented Nov 20, 2017

We have already a method that requires only to pass Encodable and if you don't worry about your JSONEncoder (which is probably the case most of the times), it is tedious to pass an empty instance to the method all the time (we have it already e.g. request with/without parameters/encoding etc.). Thus, I'm more keen to just add another case to our Task type.

Thoughts @evgeny-sureev @SD10 @AndrewSB?

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 20, 2017

Member

@sunshinejr I agree that adding another case best matches the existing request/parameter encoding API. It doesn’t make sense to treat encodable requests any different. If we’re not going to require users to specify parameter encoding on every request then we shouldn’t require them to pass a JSON decoder (which is more likely to remain constant).

Sent with GitHawk

Member

SD10 commented Nov 20, 2017

@sunshinejr I agree that adding another case best matches the existing request/parameter encoding API. It doesn’t make sense to treat encodable requests any different. If we’re not going to require users to specify parameter encoding on every request then we shouldn’t require them to pass a JSON decoder (which is more likely to remain constant).

Sent with GitHawk

@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 20, 2017

Contributor

@sunshinejr I think adding new case to Task, as I did earlier, is better.

Contributor

evgeny-sureev commented Nov 20, 2017

@sunshinejr I think adding new case to Task, as I did earlier, is better.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Nov 28, 2017

Member

Cool! @evgeny-sureev can we go back to two cases then? :)

Oh, and btw, I changed your base branch to development as per our updated Development process docs.

Member

sunshinejr commented Nov 28, 2017

Cool! @evgeny-sureev can we go back to two cases then? :)

Oh, and btw, I changed your base branch to development as per our updated Development process docs.

@sunshinejr sunshinejr changed the base branch from master to development Nov 28, 2017

@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 28, 2017

Contributor

Ok, now we have two cases.

Contributor

evgeny-sureev commented Nov 28, 2017

Ok, now we have two cases.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 28, 2017

Member

@evgeny-sureev Thanks 👍 It looks good to me. If you could just two do more things:

  1. Fix the SwiftLint warning
  2. Add a CHANGELOG entry under Added, this is a breaking change
Member

SD10 commented Nov 28, 2017

@evgeny-sureev Thanks 👍 It looks good to me. If you could just two do more things:

  1. Fix the SwiftLint warning
  2. Add a CHANGELOG entry under Added, this is a breaking change
@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 28, 2017

Contributor

Do you have an idea how to fix cyclomatic complexity warning? Other warnings are not a result of this pull request.

Contributor

evgeny-sureev commented Nov 28, 2017

Do you have an idea how to fix cyclomatic complexity warning? Other warnings are not a result of this pull request.

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 28, 2017

Member

@evgeny-sureev I don't see a good way to fix that one. When I made my comment Moya had not updated the comment from the results of CI. The build was failing so I restarted it

Member

SD10 commented Nov 28, 2017

@evgeny-sureev I don't see a good way to fix that one. When I made my comment Moya had not updated the comment from the results of CI. The build was failing so I restarted it

@evgeny-sureev

This comment has been minimized.

Show comment
Hide comment
@evgeny-sureev

evgeny-sureev Nov 28, 2017

Contributor

Maybe add '// swiftlint:disable:next cyclomatic_complexity'?

Contributor

evgeny-sureev commented Nov 28, 2017

Maybe add '// swiftlint:disable:next cyclomatic_complexity'?

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Nov 28, 2017

Member

@evgeny-sureev Sure that would be fine by me. It looks like there are 39 failing tests on CI

Member

SD10 commented Nov 28, 2017

@evgeny-sureev Sure that would be fine by me. It looks like there are 39 failing tests on CI

@@ -286,6 +325,26 @@ final class EndpointSpec: QuickSpec {
}
}
context("when JSONEncoder set with incorrect parameters") {

This comment has been minimized.

@sunshinejr

sunshinejr Nov 29, 2017

Member

nice one 👌

@sunshinejr

sunshinejr Nov 29, 2017

Member

nice one 👌

@sunshinejr

sunshinejr approved these changes Nov 29, 2017 edited

This looks good to me! Thank you for your hard work, @evgeny-sureev! Would love one more review before merging, though. cc @SD10

@SD10

SD10 approved these changes Nov 29, 2017

LGTM 👍 I'm going to tag #1357 so that we can update docs/Targets.md in the Chinese documentation

@SD10 SD10 merged commit 6bfca04 into Moya:development Nov 29, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Nov 29, 2017

Member

Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions.

Member

ashfurrow commented Nov 29, 2017

Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions.

@BasThomas BasThomas referenced this pull request Nov 29, 2017

Open

Changes in English docs not reflected in Chinese docs #1357

6 of 13 tasks complete
@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Nov 30, 2017

Member

Uggh, I don't particularly like the trade-off we made, but I understand why we did. Thank you for your work and patience @evgeny-sureev 👍

Member

AndrewSB commented Nov 30, 2017

Uggh, I don't particularly like the trade-off we made, but I understand why we did. Thank you for your work and patience @evgeny-sureev 👍

@evgeny-sureev evgeny-sureev deleted the evgeny-sureev:add-custom-jsonencoder branch Nov 30, 2017

@SD10 SD10 added this to the 11.0 milestone Jan 11, 2018

@SD10 SD10 referenced this pull request Feb 8, 2018

Merged

[Release] Moya 11.0 #1568

@Lang-le

This comment has been minimized.

Show comment
Hide comment
@Lang-le

Lang-le Jul 19, 2018

@evgeny-sureev How can i send url parameters with .requestCustomJSONEncodable? Please help!

Lang-le commented Jul 19, 2018

@evgeny-sureev How can i send url parameters with .requestCustomJSONEncodable? Please help!

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Jul 19, 2018

Member

@Lang-le Your question is better suited to opening a new GitHub issue

Member

SD10 commented Jul 19, 2018

@Lang-le Your question is better suited to opening a new GitHub issue

@Lang-le

This comment has been minimized.

Show comment
Hide comment
@Lang-le

Lang-le Jul 19, 2018

@SD10 Thank you! I will open it!

Lang-le commented Jul 19, 2018

@SD10 Thank you! I will open it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment