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

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?

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?

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?

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

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

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

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 ?

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

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

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)

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.

@Anto95 Do you still have concern?

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

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.