Rework default date #51
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows the date to be pulled from an APOD page. I also replaced the request for today's date + fallback to yesterday with a request to astropix.html + date parsing.
date will be None if parameter is not specified, passing None down makes _get_apod_chars use astropix.html instead of the date-formatted page (this url will default to the most recent)
I realized this error handling seems to be needed for requesting random dates, so I left it in with the added check that dt must not be None (when checking if use_default_today_date is True)
|
This pull request introduces 1 alert when merging 97aa47a into a434768 - view on LGTM.com new alerts:
|
|
Anyway this can mess up someone else's code that uses this API because the assumed behavior has changed??? Thoughts? |
|
I suppose it could. Looking at the source code, though, it seems the API's intended behavior was for it to do this, since it tries to fall back to the previous date if it fails to retrieve the current one. I implemented this because it seemed that this functionality was intended, but not working properly. |
|
Okay, I'll merge. It won't get deployed until later today or perhaps Monday. |
This was referenced Mar 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the
&date=parameter is left out of the query, the API currently defaults todatetime.datetime.today().date(), which is essentially "today UTC". This works, usually, but starting at 00:00 UTC until the next APOD image is uploaded, this API will 404 when requesting the current APOD image.I saw that the API has a fallback to yesterday's date if today doesn't work, but for some reason this fallback either isn't helping or isn't actually implemented in production. I chose to assume it was implemented in production, but wasn't helping. This was my fix for the issue, under that assumption.
I implemented the
_datehelper function so, when a user makes a request without specifyingdate, instead of using the datetime module's implementation of "today", makes a request to https://apod.nasa.gov/apod/astropix.html, which will be the most recent valid APOD image. This essentially means the default is handled by apod.nasa.gov, so the API shouldn't 404 anymore when thedateparameter is left out of the API query.My function processes the HTML returned by apod.nasa.gov and, similarly to how the other helper functions work, returns a datetime.date object created from the date detected on that page. This is the code that actually allows a request to be made to astropix.html instead of the standard formatted date string, while still returning the picture's upload date in the JSON data.