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

Add Flight api skeleton #85

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add Flight api skeleton #85

wants to merge 8 commits into from

Conversation

YourFin
Copy link
Collaborator

@YourFin YourFin commented Jan 8, 2020

Provides a start on #27 (and to some extent #28). I'm mostly looking for feedback here on potential additional information that would be needed for an MVP, or if there is preference for a different interface in the code.

@AlexanderOtavka AlexanderOtavka temporarily deployed to ride-board-flight-api-umvllcak January 8, 2020 22:05 Inactive
@AlexanderOtavka AlexanderOtavka added the DO NOT MERGE Waiting for more discussion/changes label Jan 8, 2020
Copy link
Owner

@AlexanderOtavka AlexanderOtavka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a reasonable interface to me, but maybe we could combine these methods into one flight search method, where you can optionally specify

  • Airline
  • Airport
  • Flight number
  • Date

and it would give you a list of matching flights that we could show to the user and let them pick.

Comment on lines 10 to 12
# Caller note: there do not appear to be guarantees that there will only be one
# flight for a given flight number, so we should be prepared to handle the case
# where we get multiple back
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple results is fine. I would imagine entering a date and flight number as more of a search operation, and then we let the user pick out their flight from a list (possibly with length 1 for an exact match or 0 for not found)

# Possible results: ApiError, [Flight]
#
# Uses same api endpoint as find_flight_by_number
def find_flight_by_airport
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the parameters here? Airport + airline + date?

# and hopefully a flightaware faFlightID we can use to keep track of the flight
# in the future
#
# Possible results: ApiError, FlightNotFound, NonEmpty [Flight]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are "FlightNotFound, NonEmpty [Flight]" separate options? "FlightNotFound" is just the bottom case of a list.

@AlexanderOtavka AlexanderOtavka temporarily deployed to ride-board-flight-api-irrqcjab January 9, 2020 06:59 Inactive
@AlexanderOtavka AlexanderOtavka had a problem deploying to ride-board-flight-api-irrqcjab January 9, 2020 06:59 Failure
@AlexanderOtavka AlexanderOtavka had a problem deploying to ride-board-flight-api-irrqcjab January 9, 2020 21:43 Failure
@YourFin
Copy link
Collaborator Author

YourFin commented Jan 9, 2020

@AlexanderOtavka the environment variables in heroku will need to be updated for this to work

@AlexanderOtavka AlexanderOtavka had a problem deploying to ride-board-flight-api-irrqcjab January 10, 2020 00:13 Failure
@AlexanderOtavka AlexanderOtavka had a problem deploying to ride-board-flight-api-irrqcjab January 10, 2020 01:00 Failure
@AlexanderOtavka AlexanderOtavka temporarily deployed to ride-board-flight-api-irrqcjab January 10, 2020 01:07 Inactive
@AlexanderOtavka
Copy link
Owner

Just added the environment variables to this review app.

This is just a reminder for myself:

DON'T FORGET TO ADD THE KEYS TO THE WHOLE PIPELINE

@AlexanderOtavka AlexanderOtavka temporarily deployed to ride-board-flight-api-irrqcjab January 11, 2020 21:45 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Waiting for more discussion/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants