search

cubehouse / themeparks

Unofficial API for accessing ride wait times and schedules for Disneyland, Disney World, Universal Studios, and many more parks
MIT License
536 stars 126 forks source link

Disney Parks Scheduled Days to Return cleanup #293

Closed cmlara closed 2 years ago

cmlara commented 4 years ago

Correct bugs around returning the selected number of days for park schedules discovered under #278

cubehouse commented 4 years ago

This looks great! Can you review how we're passing in scheduleDaysToReturn and simplify it a little.

cmlara commented 4 years ago

Hello cubehouse,

Doing some quick checks it looks like I can simplify it to be just "options.scheduleDaysToReturn = options.scheduleDaysToReturn || 30" without causing any easily visible repercussions down stream if we are ok with continuing no verification checks on the initialization data.

I had added the extra complexity to at handle a caller passing in a value larger than what Disneyland will return actual valid data for (while data exists in the database cache its always been 'closed' for dates outside the 30 day window from what I could see) though I'll admit no similar check exists on the WDW resort at this time and my checks as written would still allow some flaws such as negative timeframes (like -1) to still pass through the same as they will pass through on the WDW resort.

cubehouse commented 4 years ago

I prefer to leave the defaults as valid and generally sane, and allow people to enter any value they wish if they wish to deal with the consequences. The park may suddenly accept values larger than 30, or indeed less than 0 (for some unpredictable events where this makes sense, who knows?).

cmlara commented 4 years ago

Ok I will resubmit with the simplified version of the code for scheduleDaysToReturn.

Additionally while I was thinking about dates less than 1 (and how they would never work with how the function is currently written) I realized there might be a small race condition inside BuildOpeningTimesResponse()

It is possible for a number of the lines of code in that function (but the biggest impact is at line 205/206 where we can end up with scheduleDaysToReturn + 1 ) to cause a race condition around "today" if the code ends up executing across a date border. Any objection to while I'm in there to making something like "today = Moment().tz(this.timzone)" and making everything else in that function run off a today.clone() instead of initializing a new Moment() based on now()?