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(addLimitsMaxPeriod): limit start_date to its minimal allowed value #1286

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Anto95
Copy link

@Anto95 Anto95 commented Jan 8, 2023

Limit start_date to its minimal allowed value depending on the interval when period is set to max.
It was done with the use case of interval=1m, this works for all intervals

…value depending on the interval when period is set to max
@ValueRaider
Copy link
Collaborator

ValueRaider commented Jan 8, 2023

What code does is different to title. I expected this:
start = max(start, minimum_allowed_start)
but code is doing this:

if start is None:
  start = minimum_allowed_start

Btw, I think code can be simplified by doing this instead (only tested on one ticker):

params = {"period2": end}
if start is not None:
  params["period1"] = start

@Anto95
Copy link
Author

Anto95 commented Jan 8, 2023

I disagree with the first part. The code already sets start to a lower value in case of a known interval, so it is like:
start = max(start, minimum_allowed_start)
under the condition the interval is "1m".

I don't get the second part of your comment, we are talking about a piece of code to be added to the if start is None clause, can you explain yourself further please?

@ValueRaider
Copy link
Collaborator

First part - just me being pedantic, not really important. Your change doesn't prevent user from providing start date before minimum allowed. E.g. history(interval='1m', start='2022-01-01')

Second part - I've pushed a change to explain clearly, do you see any different behaviour?

@Anto95
Copy link
Author

Anto95 commented Jan 8, 2023

I am still confused.
Why would you put a if start is not None: after a if/else close that defines start in cases when it isn't defined? And how would this be a simplification?

Also, I am not sure to understand, if you comment out the lines with the limits, how are the limits applied?

@ValueRaider
Copy link
Collaborator

Because start doesn't need to be defined, it turns out (I think with minimal testing). Let Yahoo handle minimum start date.

@Anto95
Copy link
Author

Anto95 commented Jan 8, 2023

They have default ranges that are smaller than the max allowed:
https://query2.finance.yahoo.com/v8/finance/chart/CAN?period2=1673188119&interval=1h&includePrePost=False&events=div%2Csplits%2CcapitalGains

Setting interval to 1h the max range allowed is 730 days, but yahoo set this by default to 60

@ValueRaider
Copy link
Collaborator

Ah I gotcha. I've rolled back my commit, you might need to re-clone branch.

@Anto95
Copy link
Author

Anto95 commented Jan 8, 2023

Thanks for taking time to go over it.
I am new to this repo. How do reviews generally go? Typically who says merge / don't merge and in how much time ?

@ValueRaider
Copy link
Collaborator

ValueRaider commented Jan 8, 2023

I'm only one actually approving merges currently, and a few others sometimes check in to comment/object. Not enough given size of userbase but 🤷

Rough process is:

  • satisfy myself with PR
  • leave PR for a few days, allowing community to comment/object. Longer for major changes
  • merge into dev branch for real-world testing
  • eventually merge into main -> PIP

@ValueRaider
Copy link
Collaborator

I've added tests, that picked up some bugs that I fixed. If happy I will merge.

@Anto95
Copy link
Author

Anto95 commented Jan 13, 2023

I understand your fix make it work in cases when end is in the future. Wouldn't it be better to prevent end to be in the future?
I got it for the second part (the + 5 seconds)

@ValueRaider
Copy link
Collaborator

ValueRaider commented Jan 13, 2023

It should handle any end date. I added start_limits to handle the 1m interval, Yahoo treats it different.

Are you familiar with test-driven development (TDD)? If you think a specific scenario should be handled a specific way, implement it in a unit test.

@ValueRaider
Copy link
Collaborator

@Anto95 Do you still have concern?

@Anto95
Copy link
Author

Anto95 commented Feb 8, 2023

Hello @ValueRaider , no more concern on my side, thanks for your explanation.

@ValueRaider
Copy link
Collaborator

ValueRaider commented Feb 9, 2023

I just noticed you're targeting main branch. Can you rebase to dev (remember to force push) then change target to dev. You don't need to re-submit pull request, GitHub is smart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants