Closed johnnybutler7 closed 1 year ago
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?
Merging #151 (81b1d66) into main (7926fc4) will decrease coverage by
0.02%
. The diff coverage is100.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%) |
:arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
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.
https://github.com/q9f/eth.rb/issues/129