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

Disney Parks Scheduled Days to Return cleanup #293

Closed
wants to merge 4 commits into from

Conversation

cmlara
Copy link
Contributor

@cmlara cmlara commented Jun 14, 2020

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

@cubehouse
Copy link
Owner

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

@cmlara
Copy link
Contributor Author

cmlara commented Jun 15, 2020

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
Copy link
Owner

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
Copy link
Contributor Author

cmlara commented Jun 16, 2020

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()?

@cubehouse cubehouse closed this Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants