Skip to content
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

Update AccessTokenPlugin #1611

Merged
merged 11 commits into from Apr 10, 2018

Conversation

Projects
None yet
8 participants
@SeRG1k17
Copy link

SeRG1k17 commented Mar 25, 2018

I think this will add some functionality to the plugin, since it's not all types of authorization header. In my current project, the title was: "Authorization: token "
Also, I added this case to the tests and made a small refactoring of this file.

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Mar 25, 2018

I am new to the topic of creating and contribute in the side of the library(frameworks). This project contains a lot of dependencies on other frameworks(Alamofire, Result, etc.) , what do I need to do to correctly compile and test Moya?

@MoyaBot

This comment has been minimized.

Copy link

MoyaBot commented Mar 25, 2018

1 Warning
⚠️ Consider also updating the Chinese docs. For Chinese translations, request the modified file(s) to be added to the list here for someone else to translate, if you can’t do so yourself.

SwiftLint found issues

Warnings

File Line Reason
AccessTokenPlugin.swift 80 Lines should not have trailing whitespace.

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 25, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (development@66006b3). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             development    #1611   +/-   ##
==============================================
  Coverage               ?   87.82%           
==============================================
  Files                  ?        5           
  Lines                  ?      156           
  Branches               ?        0           
==============================================
  Hits                   ?      137           
  Misses                 ?       19           
  Partials               ?        0

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 66006b3...8884b97. Read the comment docs.

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Mar 25, 2018

what do I need to do to correctly compile and test Moya?

@SeRG1k17 Thanks for contributing and welcome 😃 You will need Carthage and run carthage bootstrap to build the dependencies.

About this PR:

We have another PR open that does what you're requesting here, see #1521. Unfortunately, we haven't had enough Moya contributors to review and decide if this is something we want to merge.

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Mar 26, 2018

@SD10 My pull request contains a lot less changes then #1521. And it can be the subpart of that request.
Although, having looked at it, I had some doubts

@BasThomas

This comment has been minimized.

Copy link
Contributor

BasThomas commented Mar 26, 2018

You will need Carthage and run carthage bootstrap to build the dependencies.

Do we have this documented somewhere?

Although, having looked at it, I had some doubts

Can you elaborate?

@sunshinejr

This comment has been minimized.

Copy link
Member

sunshinejr commented Mar 26, 2018

@BasThomas you even wrote this one 😄 Just that there is a specific command to update the libraries if possible, where bootstrap just runs on defaults. We could improve it by adding a flag to cache libraries, though.

About this PR we will have to decide what to do, because we already have a PR in the works, which is a bigger rewrite. This one is simpler and doesn't require that much to migrate for the old users.


switch self {
case .none: return ""
case .basic, .bearer: return String(describing: self).capitalized

This comment has been minimized.

@SD10

SD10 Mar 26, 2018

Member

Can you just use a String literal "Basic" here please? I trust you not to misspell it 😝

This comment has been minimized.

@BasThomas

BasThomas Mar 26, 2018

Contributor

But it should support .basic and .bearer right? What about self.rawValue.capitalized?

This comment has been minimized.

@SD10

SD10 Mar 26, 2018

Member

We have to split it into 2 separate cases. We lost rawValue because we have an associated value now. The reason I suggested this is I don't want to rely on String(describing:) then capitalized to save 1 line of code.

This comment has been minimized.

@sunshinejr

sunshinejr Mar 26, 2018

Member

I agree, never really trusted String(describing:) 😅 You just miss the fact that the value is optional and the returned string is not something you'd expect! 2 separate cases seems reasonable to me 👍

/// Custom header implementation.
case custom(String)

public var value: String {

This comment has been minimized.

@SD10

SD10 Mar 26, 2018

Member

I think this makes more sense to be optional and return nil for .none. We just break on .none so having a non-nil value doesn't save us much. It just makes it more confusing for a user that they are receiving an empty string.

This comment has been minimized.

@SD10

SD10 Mar 26, 2018

Member

Scratch that, ValidationType returns an empty array for .none so I think this is fine as well. Better to remain consistent

This comment has been minimized.

@sunshinejr

sunshinejr Mar 26, 2018

Member

I think that returning optional value is good though. Right now you need to have the knowledge that the value is useless for .none case. With optional value you'll always get a meaningful value from it, if you don't, you'll get nil.

It is kinda different with ValidationType, because you'll either way check if the statusCode is in the returning array. In this case you will need an additional conditional statement, that if the string is empty, do not do anything.
But yeah we could improve the ValidationType in the future if needed.

This comment has been minimized.

@SD10

SD10 Mar 26, 2018

Member

That’s a great point! You rarely if ever see optional arrays for that reason. I change my mind again 😆

@SD10 SD10 changed the base branch from master to development Mar 26, 2018

@sunshinejr

This comment has been minimized.

Copy link
Member

sunshinejr commented Mar 26, 2018

@SeRG1k17 would you please remove the @autoclosure from the plugin as well? We agreed to remove it as it causes a lot of chaos, where we just wanted to make it easier for people to pass the token 😅

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Mar 27, 2018

@sunshinejr @SD10 Also I see two completely different approaches to improvement:

  1. Add token property to protocol.
public protocol AccessTokenAuthorizable {

    /// Represents the authorization header to use for requests.
    var authorizationType: AuthorizationType { get }
    var token: String? { get }
}

Then, remove init from plugin and get token in prepare(_:target:) like as:

let token = authorizable.token
  1. Rework init closure as:
    public init(tokenClosure: @escaping (TargetType) -> String) {
        self.tokenClosure = tokenClosure
    }

Then get token in prepare(_:target:) like as:

let token = tokenClosure(target)

OR
Rework plugin protocol, add associatedtype:

public protocol PluginType {
    
    associatedtype Target: TargetType
    
    /// Called to modify a request before sending.
    func prepare(_ request: URLRequest, target: Target) -> URLRequest
//...

public struct AccessTokenPlugin<Target: TargetType>: PluginType {//...}
}

This will not receive in closure a protocol object, but a specific type.
However, this change will require updating the code of all the plug-ins.

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Mar 28, 2018

@SeRG1k17 I don't think we should focus on improving the AccessTokenPlugin further in this PR. That was the goal of #1521. I think we should keep it simple and only add the ability to set a custom header since this is what is most commonly requested.

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Mar 30, 2018

@SeRG1k17 Are you able to make the changes described above?

  • Make the value property return an optional String
  • Use a hardcoded string value instead of String(describing:)
  • Fix SwiftLint warnings

We also need to:

  • Add a CHANGELOG entry
  • Update the docs
@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 3, 2018

@SeRG1k17 Any update on this? I'd like to get it in the release of Moya + Swift 4.1. If you like I can take over from here.

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Apr 4, 2018

@SD10 I will try to do everything necessary today

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 4, 2018

@SeRG1k17 Doesn't need to be that rushed. If we could get this done by the end of the weekend that would be 👌 If you need any help just LMK!

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Apr 7, 2018

@SD10 I think it's ready. Sorry for the delay, very busy now.

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 8, 2018

Lot of excess commits here... seems like commits are from master that are not yet in development.
cc @sunshinejr what should we do?

Rebase this or open a PR syncing master & development and see what's left here?

@sunshinejr

This comment has been minimized.

Copy link
Member

sunshinejr commented Apr 8, 2018

@SD10 yeah, tough one. I'd try merging master into development as we need to do it anyways. If it doesn't help, then let's rebase the PR.

@SD10 SD10 changed the title Update AccessTokenPlugin Update AccessTokenPlugin Apr 8, 2018

@SD10 SD10 changed the base branch from development to master Apr 8, 2018

@SD10 SD10 changed the base branch from master to development Apr 8, 2018

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 8, 2018

Don't think I can fix this one with my git-Fu skills. Trying to drop the merge commits but they don't show up in interactive rebase.

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Apr 8, 2018

@SD10 I merged it. It`s correct?

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 8, 2018

@SeRG1k17 Sorry this one has been so confusing. We have a really messy git history too... I would've preferred we did a rebase instead of merging the branch multiple times.

Can you try to squash/clean up the history?

It looks like the CHANGELOG is corrupted now too 😓 I could possibly take over later if this is too much for you?

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Apr 8, 2018

@SD10 I need make rebase from Moya/development to my/development. Right?
And add all my changes again?

@sunshinejr sunshinejr force-pushed the SeRG1k17:development branch from dd929bf to 4c3d3a7 Apr 9, 2018

@sunshinejr

This comment has been minimized.

Copy link
Member

sunshinejr commented Apr 9, 2018

Hey guys @SeRG1k17 @SD10 - I've rebased the PR and did reset some comments, it seems like the history is little bit more clear now and we can proceed 😉

@SD10

SD10 approved these changes Apr 9, 2018

Copy link
Member

SD10 left a comment

LGTM 👍 Thank you @SeRG1k17 @sunshinejr

@SD10

This comment has been minimized.

Copy link
Member

SD10 commented Apr 9, 2018

This will require an update to docs_CN/Authentication.md so I'll tag #1357

@BasThomas BasThomas referenced this pull request Apr 9, 2018

Open

Changes in English docs not reflected in Chinese docs #1357

6 of 13 tasks complete
@sunshinejr
Copy link
Member

sunshinejr left a comment

Thanks again for the works @SeRG1k17, really happy we finished this one, was on a long waiting list 🧡

@sunshinejr sunshinejr merged commit 9d22672 into Moya:development Apr 10, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

ashfurrow commented Apr 10, 2018

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.

@SeRG1k17

This comment has been minimized.

Copy link
Author

SeRG1k17 commented Apr 11, 2018

@ashfurrow Thanks a lot. I will try to help develop this project. A few new ideas are already on the way :)

@ashfurrow ashfurrow referenced this pull request Apr 11, 2018

Closed

Migrate from Aeryn #15

@oluckyman

This comment has been minimized.

Copy link

oluckyman commented May 2, 2018

Awesome!
Looking forward to see it merged into master

@sunshinejr

This comment has been minimized.

Copy link
Member

sunshinejr commented May 2, 2018

@oluckyman this needs a major version release as it's breaking, but I think we are near to complete the scope for it 😉

@SD10 SD10 referenced this pull request Nov 2, 2018

Merged

[Release] Moya 12.0 #1753

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.