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

Feature/add http basic support auth #151

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

johnnybutler7
Copy link
Contributor

@q9f q9f added the enhancement New feature or request label Oct 27, 2022
@q9f
Copy link
Owner

q9f commented Oct 27, 2022

Thank you! My only concern is that it duplicates some code from the HTTP client. What about adding a conditional check for user and password to the HTTP client?

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Merging #151 (81b1d66) into main (7926fc4) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
- Coverage   99.74%   99.72%   -0.03%     
==========================================
  Files          67       68       +1     
  Lines        3923     3968      +45     
==========================================
+ Hits         3913     3957      +44     
- Misses         10       11       +1     
Impacted Files Coverage Δ
lib/eth/client.rb 100.00% <100.00%> (ø)
lib/eth/client/http_basic.rb 100.00% <100.00%> (ø)
spec/eth/client_spec.rb 100.00% <100.00%> (ø)
lib/eth/client/ipc.rb 93.33% <0.00%> (-6.67%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@johnnybutler7
Copy link
Contributor Author

Thank you! My only concern is that it duplicates some code from the HTTP client. What about adding a conditional check for user and password to the HTTP client?

Thanks for your reply. Initially I started off with the approach but the code became a bit ugly. Generally I find creating a whole new thing is a better trade off in this situation even if it duplicates some code. They don't share all the same attributes(user, password) so its different enough from Client::Http to warrant a dedicated class of its own Client::HttpBasic

Could potentially create a subclass of Client::HttpBasic with the different functionality but that's a bit overkill for this.

@q9f q9f merged commit 003dbe8 into q9f:main Oct 31, 2022
a-moreira pushed a commit to a-moreira/eth.rb that referenced this pull request Oct 31, 2022
* Adds support for basic http auth

* Adds support for basic http auth
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