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

Simplify headers handling #295

Merged
merged 3 commits into from Jan 27, 2022
Merged

Simplify headers handling #295

merged 3 commits into from Jan 27, 2022

Conversation

brunoocasali
Copy link
Member

This improvement creates a new attr to be instantiated during the initializer phase and after the instantiation, it could not be updated directly, just copies of it using dup.
This should simplify the headers handling since they were mixed with options.

Originally extracted from #292

This improvement creates a new attr to be instantiated during the initializer
and after the instantiation it could not be updated directly, just copies of it.

The general idea was to remove some helper methods `clone_options` and `merge_options`
that only had a purpouse because we have been handling the headers inside a hash (options hash).
This extra complexity makes the requirement to have all these internal structures
in order to be possible to add/remove and modify the items inside of it.

After this we should only update a clone when it's required like was did in the lines 30, 41, 72.
* Avoid parameter lists longer than 5 parameters. [6/5]
@brunoocasali brunoocasali added the enhancement New feature or request label Jan 26, 2022
def build_default_options_headers
{
'Content-Type' => 'application/json',
'Authorization' => ("Bearer #{@api_key}" unless @api_key.nil?)
Copy link
Member Author

Choose a reason for hiding this comment

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

That I was trying to show to you @curquiza that day #273 comment 😅

Copy link
Member

Choose a reason for hiding this comment

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

Sooooooo cool!

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

👌

Thanks brubru!!

@brunoocasali
Copy link
Member Author

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Jan 27, 2022

@meili-bors meili-bors bot merged commit 4d43a12 into main Jan 27, 2022
@meili-bors meili-bors bot deleted the simplify-headers-handling branch January 27, 2022 12:32
meili-bors bot added a commit that referenced this pull request Feb 1, 2022
296: Update version for the next release (v0.18.1) r=brunoocasali a=brunoocasali

Release a new version with the following changes:

- #292 
- #290 
- #295 

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
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.

None yet

2 participants