Add historical prices api endpoint #94

tylerhaugen-stanley · 2020-11-07T23:33:11Z

No description provided.

dblock · 2020-11-08T15:52:44Z

Looks good, will merge on green.

…ces-endpoint

dblock · 2020-11-10T15:10:05Z

lib/iex/endpoints/historial_prices.rb

+      def historical_prices(symbol, options = {})
+        slug = "stock/#{symbol}/chart"
+        slug += "/#{options[:range]}" if options[:range]
+        slug += "/#{options[:date]}" if options[:range] == 'date'


This will be wrong if options[:date] is missing. I would add a check to raise ArgumentError on a missing date when range is date or something like that. Make sure to add specs for the combinations.

Technically this will just return an empty array but that's pretty useless so I'll add the argument error.

Good call about the specs, pretty sloppy of me. While adding specs I noticed that you can actually receive a different payload if you ask for a specific date and don't include additional query params (ex: chartByDay: true). This will take a bit of time to investigate how to best solve so It'll be a bit before this PR is ready for review again.

Don't let this block you.

Sounds good. Basically you can get 2 types of data from the IEX endpoint, per day or per min data. The current implementation only supports the per day format.

dblock · 2020-11-10T15:10:43Z

lib/iex/endpoints/historial_prices.rb

+
+        # We only want options to include query params at this point, remove :range and :date
+        options.delete(:range)
+        options.delete(:date)


You're modifying the hash passed in, so either options = options.dup, or use .except or similar to get a copy without those keys. Let's make sure there are no other places where we did this, I may have missed that.

I didn't see any downside to modifying it but happy to dup it if you prefer. This was the only place I did this.

The downside is that code like this doesn't do what you'd expect it to do:

options = { range: ... } prices_for_msft = historical_prices('msft', options) prices_for_goog = historical_prices('goog', options)

Also use options.key?(:range), nil is a value and while it doesn't make sense you don't want the caller to rely on you clearing the option, this is more by convention

Yup that's a downside. Thanks

dblock · 2020-11-10T15:10:58Z

lib/iex/endpoints/historial_prices.rb

+  module Endpoints
+    module HistoricalPrices
+      def historical_prices(symbol, options = {})
+        slug = "stock/#{symbol}/chart"


This is a path rather than a slug, let's call it path.

tylerhaugen-stanley · 2020-11-11T16:25:01Z

lib/iex/resources/resource.rb

@@ -14,6 +14,18 @@ def self.float_to_percentage(float_number)
        ].join
      end

+      # Useful for values that are already a percent but we want to convert into a 2 decimal place string
+      def self.percentage_to_string(float_percent)


This is essentially a direct copy of float_to_percentage but the the values being returned from IEX for historical prices were already a percent just in float format, so the string representation ended up being an incorrect value. Happy to try and combine these two methods with an optional parameter or something different? Don't love the copy paste nature of this.

I think copy paste is fine I wouldn't worry about it.

tylerhaugen-stanley · 2020-11-13T01:50:00Z

I think I've addressed all your concerns @dblock, please let me know if there are more. Also not sure why the travis-ci build failed, I don't see anything in the details page.

dblock · 2020-11-13T15:50:41Z

1 Error
🚫 The TOC found in README.md doesn't match the sections of the file.
Here's the expected TOC for README.md:

# Table of Contents

- [Installation](#installation)
- [Usage](#usage)
  - [Get an API Token](#get-an-api-token)
  - [Configure](#configure)
  - [Get a Single Price](#get-a-single-price)
  - [Get a Quote](#get-a-quote)
  - [Get a OHLC (Open, High, Low, Close) price](#get-a-ohlc-open-high-low-close-price)
  - [Get a Market OHLC (Open, High, Low, Close) prices](#get-a-market-ohlc-open-high-low-close-prices)
  - [Get Historical Prices](#get-historical-prices)
  - [Get Company Information](#get-company-information)
  - [Get a Company Logo](#get-a-company-logo)
  - [Get Recent News](#get-recent-news)
  - [Get Chart](#get-chart)
  - [Get Key Stats](#get-key-stats)
  - [Get Advanced Stats](#get-advanced-stats)
  - [Get Dividends](#get-dividends)
  - [Get Earnings](#get-earnings)
  - [Get Income Statement](#get-income-statement)
  - [Get Balance Sheet](#get-balance-sheet)
  - [Get Cash Flow Statement](#get-cash-flow-statement)
  - [Get Sector Performance](#get-sector-performance)
  - [Get Largest Trades](#get-largest-trades)
  - [Get a Quote for Crypto Currencies](#get-a-quote-for-crypto-currencies)
  - [ISIN Mapping](#isin-mapping)
  - [Get Symbols](#get-symbols)
  - [Get List](#get-list)
  - [Other Requests](#other-requests)
- [Configuration](#configuration)
  - [Logging](#logging)
- [Sandbox Environment](#sandbox-environment)
- [Errors](#errors)
  - [SymbolNotFound](#symbolnotfound)
  - [PermissionDeniedError](#permissiondeniederror)
  - [ClientError](#clienterror)
- [Contributing](#contributing)
- [Copyright and License](#copyright-and-license)

Generated by 🚫 Danger

The build is failing because of this ^, fix the TOC.

dblock · 2020-11-13T15:51:16Z

README.md

@@ -15,6 +15,7 @@ A Ruby client for the [The IEX Cloud API](https://iexcloud.io/docs/api/).
  - [Get a Quote](#get-a-quote)
  - [Get a OHLC (Open, High, Low, Close) price](#get-a-ohlc-open-high-low-close-price)
  - [Get a Market OHLC (Open, High, Low, Close) prices](#get-a-market-ohlc-open-high-low-close-prices)
+  - [Get Historical Prices](#get-historial-prices)


There's a typo in historical, missing c.

Ahh I expected Danger to re-post an error after the initial one where I fixed the header but not the tag.

dblock · 2020-11-13T15:51:54Z

lib/iex/endpoints/historial_prices.rb

+    module HistoricalPrices
+      def historical_prices(symbol, options = {})
+        if options[:range] == 'date'
+          raise ArgumentError if options[:date].nil?


unless options[...] feels cleaner

Personally I prefer the if route but happy to change.

dblock · 2020-11-13T15:52:24Z

lib/iex/endpoints/historial_prices.rb

+
+        options = options.dup
+        path = "stock/#{symbol}/chart"
+        path += "/#{options[:range]}" if options[:range]


if options.key?(:range), which is faster

My bad, misunderstood your previous comment.

dblock · 2020-11-13T15:53:47Z

lib/iex/endpoints/historial_prices.rb

+        path = "stock/#{symbol}/chart"
+        path += "/#{options[:range]}" if options[:range]
+        # IEX needs dates passed without any formatting, trim out common symbols if passed in.
+        path += "/#{options[:date].tr('-/', '')}" if options[:range] == 'date'


This feels wrong. IEX expects a date in a well defined format, not one without dashes or slashes, and it's not for the client to be mangling input. It's a slippery slope when IEX adds support for -5d (last 5 days) or something like that and we have to deprecate magical behavior. So either leave the input as is, and document what's required, and/or add support for passing in a DateTime and convert it into the right kind of string.

So i did this because typing in a date as 20201101 is a lot more difficult as a user of the library than typing in 2020-11-01 or 2020/11/01 as most humans would type a date. I like the DateTime approach more through so I'll change that and the documentation.

On second thought, any specific reason for DateTime vs Date? This endpoint has no concept of time in its current implementation.

… and readme accordingly

dblock

I'm happy to merge as is, but consider these two suggestions?

rubocop -a will fix the build

dblock · 2020-11-14T20:33:14Z

spec/iex/endpoints/historical_prices_spec.rb

+
+        it 'retrieves historical prices' do
+          expect(subject.size).to eq 23
+          expect(historical_price.date).to eq '2020-10-07'


Should this be converted to a Date as well?

Yeah.... I think it should be however, all the other endpoints that return dates are also strings. I say lets leave this as is and I'll fix all of them up in a follow up PR.

For this, would you prefer two fields, one for the raw data and one for the converted data? Similar to how dollar values are handled?

Dollar values are different from dates since those are integers in cents, and formatted values in dollars. I think this should just be a Date type. NBD though, I'm going to merge as is and feel free to change it on top before the next release, double-checking that we do it everywhere a date is returned in other instances.

dblock · 2020-11-14T20:36:05Z

lib/iex/endpoints/historial_prices.rb

+      def historical_prices(symbol, options = {})
+        if options[:range] == 'date'
+          raise ArgumentError unless options[:date].present?
+          raise ArgumentError, 'Date param must be a Date object' unless options[:date].class == Date


.is_a?(Date) is the proper way to do this, for one it can inherit from Date

I think you want to support non-Date input and pass it through in that case. So I would just drop this case. When passing Date, call strftime, otherwise pass through the value as is.

options = options.dup options[:date] = options[:date].strftime('...') if options[:date].is_a?(Date)

Good point about is_a thanks.

Fair enough with passing the date through as a string. If the string date is incorrect then the call with simply error out. 👍

tylerhaugen-stanley · 2020-11-14T21:47:08Z

I'm happy to merge as is, but consider these two suggestions?

rubocop -a will fix the build

Ahh keep forgetting about rubocop. Apologies.

Add historical prices api endpoint

a8aa3d1

tylerhaugen-stanley force-pushed the add-historial-prices-endpoint branch from ec45e49 to a8aa3d1 Compare November 7, 2020 23:33

Fix typo in readme

4af2815

tylerhaugen-stanley added 2 commits November 8, 2020 12:06

Update readme to be more accurate

becd9f6

Merge remote-tracking branch 'upstream/master' into add-historial-pri…

ffb1a43

…ces-endpoint

dblock requested changes Nov 10, 2020

View reviewed changes

Update historical prices endpoint to be more fault tolerant

5736a44

tylerhaugen-stanley commented Nov 11, 2020

View reviewed changes

dblock reviewed Nov 13, 2020

View reviewed changes

dblock requested changes Nov 13, 2020

View reviewed changes

Change historical prices date param to be a Date object, update specs…

64c8266

… and readme accordingly

dblock approved these changes Nov 14, 2020

View reviewed changes

Allow historical prices endpoint to accept string or Dates in the params

1b1a0be

dblock merged commit 91d9bb2 into dblock:master Nov 14, 2020

tylerhaugen-stanley deleted the add-historial-prices-endpoint branch November 19, 2020 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add historical prices api endpoint #94

Add historical prices api endpoint #94

tylerhaugen-stanley commented Nov 7, 2020

dblock commented Nov 8, 2020

dblock Nov 10, 2020

tylerhaugen-stanley Nov 11, 2020

dblock Nov 11, 2020

tylerhaugen-stanley Nov 11, 2020

dblock Nov 10, 2020

tylerhaugen-stanley Nov 11, 2020

dblock Nov 11, 2020

dblock Nov 11, 2020

tylerhaugen-stanley Nov 11, 2020

dblock Nov 10, 2020

tylerhaugen-stanley Nov 11, 2020

dblock Nov 11, 2020

tylerhaugen-stanley commented Nov 13, 2020

dblock commented Nov 13, 2020

dblock Nov 13, 2020

tylerhaugen-stanley Nov 14, 2020

dblock Nov 13, 2020

tylerhaugen-stanley Nov 14, 2020

dblock Nov 13, 2020

tylerhaugen-stanley Nov 14, 2020

dblock Nov 13, 2020

tylerhaugen-stanley Nov 14, 2020

tylerhaugen-stanley Nov 14, 2020

dblock left a comment •

edited

Loading

dblock Nov 14, 2020

tylerhaugen-stanley Nov 14, 2020

tylerhaugen-stanley Nov 14, 2020

dblock Nov 14, 2020

dblock Nov 14, 2020

tylerhaugen-stanley Nov 14, 2020

tylerhaugen-stanley commented Nov 14, 2020

Add historical prices api endpoint #94

Add historical prices api endpoint #94

Conversation

tylerhaugen-stanley commented Nov 7, 2020

dblock commented Nov 8, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerhaugen-stanley commented Nov 13, 2020

dblock commented Nov 13, 2020

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 left a comment • edited Loading

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

tylerhaugen-stanley commented Nov 14, 2020

dblock left a comment •

edited

Loading