Skip to content

Conversation

ppamorim
Copy link
Contributor

Pull Request

What does this PR do?

Fixes issue pointed on #272

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@ppamorim ppamorim added the enhancement New feature or request label Apr 16, 2022
@ppamorim ppamorim requested a review from brunoocasali April 16, 2022 21:26
@ppamorim ppamorim self-assigned this Apr 16, 2022
@ppamorim ppamorim changed the title Refactor the function function to use a simple request builder Refactor the request function to use a simple request builder Apr 16, 2022
@ppamorim ppamorim marked this pull request as ready for review April 18, 2022 19:41
@brunoocasali brunoocasali requested a review from bidoubiwa April 20, 2022 18:28
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that only the get method takes the headers as argument.

@ppamorim I saw that you were the one that added the header as argument in the get method. Is this something on purpose or should it be added in the other methods as well?

DOn't worry I can do the PR, I was just wondering if there is a reason for this

  func get(
    api: String,
    param: String? = nil,
    headers: [String: String] = [:],

@ppamorim
Copy link
Contributor Author

@bidoubiwa I can't approve your suggestions because Github is not accepting my request.
Screenshot 2022-04-21 at 22 35 21

@bidoubiwa
Copy link
Contributor

Hey @ppamorim tell me if it fixed, if it isn't I can commit for you if you'd like to!

@ppamorim
Copy link
Contributor Author

@bidoubiwa It's not. I could not hear from Github.

@bidoubiwa
Copy link
Contributor

Okey I will make the changes asap!

@bidoubiwa bidoubiwa removed the request for review from brunoocasali May 4, 2022 13:07
bidoubiwa
bidoubiwa previously approved these changes May 5, 2022
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot 🔥

Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR @ppamorim 💪

Feel free to apply or not the suggestions I made :)

_ headers: [String: String] = [:],
data: Data? = nil
) -> URLRequest {
var request = URLRequest(url: url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    var request = URLRequest(url: url)
    request.httpMethod = httpMethod.rawValue
    request.setValue(PackageVersion.qualifiedVersion(), forHTTPHeaderField: "User-Agent")
    
    if httpMethod != .get {
      request.httpBody = data
      request.setValue("application/json", forHTTPHeaderField: "Content-Type")
    }
    
    if let apiKey: String = config.apiKey {
      let bearer = "Bearer \(apiKey)"
      request.setValue(bearer, forHTTPHeaderField: "Authorization")
    }
    
    headers.forEach { key, value in
      request.addValue(value, forHTTPHeaderField: key)
    }

    return request

I added some spaces between functions, I think they were important (it is easier to read :D).
But I changed some function execution orders as well, this will help anyone to apply custom data from the headers param when needed (idk, if to fully apply this concept we need to switch over setValue instead of addValue).

@bidoubiwa
Copy link
Contributor

bors try

@bidoubiwa bidoubiwa requested a review from brunoocasali May 16, 2022 11:36
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks for the contribution! ❤️

@bidoubiwa
Copy link
Contributor

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented May 16, 2022

Build succeeded:

@meili-bors meili-bors bot merged commit 74d09cf into main May 16, 2022
@meili-bors meili-bors bot deleted the requestRefactor branch May 16, 2022 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants