Skip to content

Conversation

@mathisbeauty
Copy link
Contributor

Implemented threshold to fetch launch dates from Launch Library API.

I attach a txt to compare both APIs rapidly.
example_queries.txt

@jakewmeyer jakewmeyer self-assigned this Aug 21, 2019
@jakewmeyer jakewmeyer self-requested a review August 21, 2019 20:23
@jakewmeyer
Copy link
Member

Just one thing to maybe consider:

  1. This assumes that the mission name will be exactly the same as what they have in LL, which might not always be the case. We might want to fuzzy match to something reasonably close.

Reduced number of requests to LL API to 1
Added negative bound to daysToLaunch to deal with undefined dates
@mathisbeauty
Copy link
Contributor Author

I have implemented the fuzzy match and also reduced the number of requests to the LL API to 1, instead of one per launch to update.

Let me know what you think.

@mathisbeauty
Copy link
Contributor Author

Also, it may be a good idea to hide the new fields 'last_wiki_update' and 'is_date_from_wiki' from the API responses.

@jakewmeyer
Copy link
Member

Might be useful to some? I'd rather keep them in the response.

Everything looks good, I'll merge this shortly.

Overall awesome work, with lots of good comments, appreciate you knocking this out for us 👍

@mathisbeauty
Copy link
Contributor Author

Thank you! Glad to help 👍
Also, I have tons of free time, so I'm thinking of contributing with another feature, like:

  • Twitter scraping for live updates
  • Making the backup APIs a list instead of just Launch Library. Having different "Providers" from which to gather the data. E.g. Twitter, other APIs, etc.
  • Writing tests for the "scripts" folder
  • Etc

Let me know which one as a higher priority, or another feature that you need help with, and I'll be glad to create an issue and start working on it.

Cheers!

@jakewmeyer jakewmeyer merged commit 8011845 into r-spacex:master Aug 23, 2019
@jakewmeyer
Copy link
Member

Twitter one should be higher priority imo. Updating the static fire times isn't automated currently.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants