Add Search endpoint #58

pmn4 · 2019-11-27T16:08:52Z

This PR adds the "Search" endpoint. Search doesn't quite fit the Resourceful pattern, so I put the tests in client_spec.rb. Happy to move them if you'd prefer them to be in their own file (I just felt strange because there is no resource to use in describe <Resource>).

I also added a new Error class, and to allow for better rescuing, created an error superclass for the gem. Also happy to revert if it doesn't feel right.

dangerpr-bot · 2019-11-27T16:10:05Z

	1 Warning
⚠️	[DEPRECATION] `check` is deprecated. Please use `check!` instead.

Generated by 🚫 Danger

pmn4

I hadn't used VCR before... thanks for the introduction! it is a fantastic tool for writing specs!

pmn4 · 2019-11-27T16:10:24Z

lib/iex/errors/symbol_not_found_error.rb

@@ -1,14 +1,27 @@
 module IEX
  module Errors
-    class SymbolNotFoundError < StandardError
-      attr_reader :symbol
+    class IexError < StandardError


added an Error superclass to allow for other projects to:

rescue IEX::Errors::IexError

instead of having to:

rescue IEX::Errors::SymbolNotFoundError, IEX::Errors::SearchNotFoundError, ... # as more errors are added

tbh, I hadn't realized there were other errors defined (I thought I was creating the second one). If I had, I probably would have put this into a separate PR. sorry for the confusion!

pmn4 · 2019-11-27T16:12:10Z

spec/iex/client_spec.rb

@@ -135,4 +135,36 @@
      end
    end
  end
+
+  context '#search' do


tests for this endpoint were added to client_spect.rb because other endpoints have associated resources, where #search (eventually, according to the documentation) returns any type of resource.

Make a directory, search and create search/quote_spec.rb and such? For the general use case of errors make a search_spec.rb.

dblock

This is great and is on the right track! See comments. Also needs README documentation.

CHANGELOG.md

lib/iex/endpoints/search.rb

lib/iex/errors/symbol_not_found_error.rb

dblock · 2019-11-27T18:43:56Z

spec/fixtures/iex/client/search/missing.yml

+http_interactions:
+- request:
+    method: get
+    uri: https://sandbox.iexapis.com/v1/search/?token=Tsk_341c973d296e4e18aa61dd63050ce235


Remove your actual token from here.

dblock · 2019-11-27T18:44:39Z

spec/iex/client_spec.rb

@@ -135,4 +135,36 @@
      end
    end
  end
+
+  context '#search' do


Make a directory, search and create search/quote_spec.rb and such? For the general use case of errors make a search_spec.rb.

dblock · 2019-11-27T18:45:12Z

spec/iex/client_spec.rb

+      # order for VCR to record the response, we need the endpoint to work
+      client.endpoint = 'https://sandbox.iexapis.com/v1'
+      # using the documentation-provided token
+      client.publishable_token = 'Tsk_341c973d296e4e18aa61dd63050ce235'


We default this to token and replace it everywhere for tests, as they are run from a recording. Don't leak your tokens!

This particular endpoint is not available to free tier accounts (the plan I'm on!), so I had to update the endpoint to the sandbox, where of course, different keys are needed. the token in this code is copy/pasted from the iexcloud docs, so it's already publicly exposed!

Is it that they have it in sandbox but not in production yet? In which case you can manually remove the sandbox part from tests and edit VCRs once you have a working recording.

spec/iex/client_spec.rb

dblock · 2019-11-27T18:46:17Z

lib/iex/endpoints/search.rb

+          'search',
+          fragment
+        ].compact.join('/'), options).map do |data|
+          # Per the IEX documentation, any type of Resource could be returned


Yes, please. And create a base resource for anything unsupported.

fyi, they have not released this feature yet, the docs just mention that it is coming

I don't see a problem supporting something in pre-release, but we have to say it in README and point to whatever IEX docs that say the same

dblock · 2020-01-06T18:54:28Z

The code here looks good. You still need to remove your actual secret token from everywhere!

dblock · 2020-01-06T18:54:47Z

Also add a comment when this is ready to re-review, commits don't get flagged in notifications.

dblock · 2020-02-01T22:48:39Z

Bump @pmn4, want to finish this?

pmn4 · 2020-02-03T21:48:44Z

@dblock there are a few items here:

Resource Type: I introduced some confusion with the search results "Resource Type." IEX has not yet launched this feature, so there is nothing to support, or even details about how to support it. The documentation says it's coming, so I left a comment mentioning that it is coming.
Tokens: the token in this PR is copy/pasted from IEX's documentation (it is their sandbox API key). I used it because the search endpoint is a paid feature, for which I do not have an API key. Instead, for this one test case, I updated both the endpoint and API key to point to the sandbox environment, so if we need to recreate the VCR response, we can without having to pay.

do these make sense? sorry for the confusion, and the delay! (tbh, I was only evaluating IEX when I authored this PR, and have since switched gears to another provider.)

dblock · 2020-02-04T14:23:31Z

@dblock there are a few items here:

Resource Type: I introduced some confusion with the search results "Resource Type." IEX has not yet launched this feature, so there is nothing to support, or even details about how to support it. The documentation says it's coming, so I left a comment mentioning that it is coming.

ok

Tokens: the token in this PR is copy/pasted from IEX's documentation (it is their sandbox API key). I used it because the search endpoint is a paid feature, for which I do not have an API key. Instead, for this one test case, I updated both the endpoint and API key to point to the sandbox environment, so if we need to recreate the VCR response, we can without having to pay.

you just don't want the real token in this PR because someone will steal it; once a recording was made you can just replace it with a placeholder

dblock · 2020-02-04T14:24:09Z

do these make sense? sorry for the confusion, and the delay! (tbh, I was only evaluating IEX when I authored this PR, and have since switched gears to another provider.)

I'll leave this open for someone/you to finish it.

pmn4 commented Nov 27, 2019

View reviewed changes

pmn4 force-pushed the master branch from 203733c to 6aea494 Compare November 27, 2019 16:50

dblock requested changes Nov 27, 2019

View reviewed changes

pmn4 force-pushed the master branch from 6aea494 to 1cd5d68 Compare November 27, 2019 19:31

Add Search endpoint

6ce653e

pmn4 force-pushed the master branch from 1cd5d68 to 6ce653e Compare November 27, 2019 19:39

bguban force-pushed the master branch from 270afff to 0ccfe9d Compare March 2, 2020 17:02

rodolfobandeira added the changes requested label Apr 11, 2020

rodolfobandeira added the hacktoberfest HacktoberFest label Oct 9, 2020

rodolfobandeira removed the hacktoberfest HacktoberFest label Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Search endpoint #58

Add Search endpoint #58

pmn4 commented Nov 27, 2019

dangerpr-bot commented Nov 27, 2019 •

edited

Loading

pmn4 left a comment

pmn4 Nov 27, 2019

pmn4 Nov 27, 2019

pmn4 Nov 27, 2019

dblock Nov 27, 2019

dblock left a comment

dblock Nov 27, 2019

dblock Nov 27, 2019

dblock Nov 27, 2019

pmn4 Nov 27, 2019

dblock Nov 29, 2019

dblock Nov 27, 2019

pmn4 Nov 27, 2019

dblock Nov 29, 2019

dblock commented Jan 6, 2020

dblock commented Jan 6, 2020

dblock commented Feb 1, 2020

pmn4 commented Feb 3, 2020

dblock commented Feb 4, 2020

dblock commented Feb 4, 2020

Add Search endpoint #58

Are you sure you want to change the base?

Add Search endpoint #58

Conversation

pmn4 commented Nov 27, 2019

dangerpr-bot commented Nov 27, 2019 • edited Loading

pmn4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Jan 6, 2020

dblock commented Jan 6, 2020

dblock commented Feb 1, 2020

pmn4 commented Feb 3, 2020

dblock commented Feb 4, 2020

dblock commented Feb 4, 2020

dangerpr-bot commented Nov 27, 2019 •

edited

Loading