-
Notifications
You must be signed in to change notification settings - Fork 93
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
Accept request URL in lieu of configuration file #728
Changes from all commits
49d04f5
be89932
f0f8591
2e0333e
2b44800
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -5,12 +5,6 @@ import Turf | |||
import FoundationNetworking | ||||
#endif | ||||
|
||||
let accessToken: String? = | ||||
ProcessInfo.processInfo.environment["MAPBOX_ACCESS_TOKEN"] ?? | ||||
UserDefaults.standard.string(forKey: "MBXAccessToken") | ||||
let credentials = Credentials(accessToken: accessToken!) | ||||
private let directions = Directions(credentials: credentials) | ||||
|
||||
protocol DirectionsResultsProvider { | ||||
var directionsResults: [DirectionsResult]? { get } | ||||
} | ||||
|
@@ -28,6 +22,7 @@ class CodingOperation<ResponseType : Codable & DirectionsResultsProvider, Option | |||
// MARK: - Parameters | ||||
|
||||
let options: ProcessingOptions | ||||
let credentials: Credentials | ||||
|
||||
// MARK: - Helper methods | ||||
|
||||
|
@@ -123,12 +118,17 @@ class CodingOperation<ResponseType : Codable & DirectionsResultsProvider, Option | |||
return interpolatedCoordinates | ||||
} | ||||
|
||||
private func requestResponse(_ directionsOptions: OptionsType) -> (Data) { | ||||
private func response(fetching directionsOptions: OptionsType) -> (Data) { | ||||
let directions = Directions(credentials: credentials) | ||||
let url = directions.url(forCalculating: directionsOptions) | ||||
return response(fetching: url) | ||||
} | ||||
|
||||
private func response(fetching url: URL) -> Data { | ||||
let semaphore = DispatchSemaphore(value: 0) | ||||
|
||||
var responseData: Data! | ||||
|
||||
let url = directions.url(forCalculating: directionsOptions) | ||||
let urlSession = URLSession(configuration: .ephemeral) | ||||
|
||||
let task = urlSession.dataTask(with: url) { (data, response, error) in | ||||
|
@@ -146,28 +146,52 @@ class CodingOperation<ResponseType : Codable & DirectionsResultsProvider, Option | |||
return (responseData) | ||||
} | ||||
|
||||
init(options: ProcessingOptions) { | ||||
init(options: ProcessingOptions, credentials: Credentials) { | ||||
self.options = options | ||||
self.credentials = credentials | ||||
} | ||||
|
||||
// MARK: - Command implementation | ||||
|
||||
func execute() throws { | ||||
|
||||
let config = FileManager.default.contents(atPath: NSString(string: options.configPath).expandingTildeInPath)! | ||||
let input: Data | ||||
|
||||
let decoder = JSONDecoder() | ||||
|
||||
let directionsOptions = try decoder.decode(OptionsType.self, from: config) | ||||
let directions: Directions | ||||
let directionsOptions: OptionsType | ||||
let requestURL: URL | ||||
if FileManager.default.fileExists(atPath: (options.config as NSString).expandingTildeInPath) { | ||||
// Assume the file is a configuration JSON file. Convert it to an options object. | ||||
let configData = FileManager.default.contents(atPath: (options.config as NSString).expandingTildeInPath)! | ||||
let decoder = JSONDecoder() | ||||
directions = Directions(credentials: credentials) | ||||
directionsOptions = try decoder.decode(OptionsType.self, from: configData) | ||||
} else if let url = URL(string: options.config) { | ||||
// Try to convert the URL to an options object. | ||||
if let parsedOptions = (RouteOptions(url: url) ?? MatchOptions(url: url)) as? OptionsType { | ||||
directionsOptions = parsedOptions | ||||
} else { | ||||
fatalError("Configuration is not a valid Mapbox Directions API or Mapbox Map Matching API request URL.") | ||||
} | ||||
|
||||
// Get credentials from the request URL but fall back to the environment. | ||||
var urlWithAccessToken = URLComponents(string: url.absoluteString)! | ||||
urlWithAccessToken.queryItems = (urlWithAccessToken.queryItems ?? []) + [.init(name: "access_token", value: self.credentials.accessToken)] | ||||
let credentials = Credentials(requestURL: urlWithAccessToken.url!) | ||||
Comment on lines
+177
to
+178
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was pretty annoying:
The code here has no opportunity to fall back to the access token from the environment, so instead it has to proactively fall back inside the URL before initializing the Credentials object. |
||||
|
||||
directions = Directions(credentials: credentials) | ||||
} else { | ||||
fatalError("Configuration is not a valid JSON configuration file or request URL.") | ||||
} | ||||
|
||||
let input: Data | ||||
if let inputPath = options.inputPath { | ||||
input = FileManager.default.contents(atPath: NSString(string: inputPath).expandingTildeInPath)! | ||||
} else { | ||||
let response = requestResponse(directionsOptions) | ||||
requestURL = directions.url(forCalculating: directionsOptions) | ||||
let response = response(fetching: requestURL) | ||||
input = response | ||||
} | ||||
|
||||
let decoder = JSONDecoder() | ||||
decoder.userInfo = [.options: directionsOptions, | ||||
.credentials: credentials] | ||||
|
||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,13 @@ import Foundation | |
import MapboxDirections | ||
import ArgumentParser | ||
|
||
|
||
struct ProcessingOptions: ParsableArguments { | ||
|
||
@Option(name: [.short, .customLong("input")], help: "[Optional] Filepath to the input JSON. If no filepath provided - will fall back to Directions API request using locations in config file.") | ||
var inputPath: String? | ||
|
||
@Option(name: [.short, .customLong("config")], help: "Filepath to the JSON, containing serialized Options data.") | ||
var configPath: String | ||
@Argument(help: "Path to a JSON file containing serialized RouteOptions or MatchOptions properties, or the full URL of a Mapbox Directions API or Mapbox Map Matching API request.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is kind of unwieldy as an argument description. A possible future improvement would be to accept multiple configurations, perhaps a mix of files and URLs, and merge them somehow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will you document that possible future improvement? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let’s track it as tail work in #729. |
||
var config: String | ||
|
||
@Option(name: [.short, .customLong("output")], help: "[Optional] Output filepath to save the conversion result. If no filepath provided - will output to the shell.") | ||
var outputPath: String? | ||
|
@@ -27,16 +26,34 @@ struct ProcessingOptions: ParsableArguments { | |
} | ||
|
||
struct Command: ParsableCommand { | ||
static var credentials: Credentials { | ||
get throws { | ||
guard let accessToken = ProcessInfo.processInfo.environment["MAPBOX_ACCESS_TOKEN"] ?? | ||
UserDefaults.standard.string(forKey: "MBXAccessToken") else { | ||
throw ValidationError("A Mapbox access token is required. Go to <https://account.mapbox.com/access-tokens/>, then set the MAPBOX_ACCESS_TOKEN environment variable to your access token.") | ||
} | ||
|
||
let hostURL: URL? | ||
if let host = ProcessInfo.processInfo.environment["MAPBOX_HOST"] ?? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of this environment variable is consistent with the Mapbox Python SDK, which similarly reads credentials from the environment. It differs from the |
||
UserDefaults.standard.string(forKey: "MGLMapboxAPIBaseURL") { | ||
hostURL = URL(string: host) | ||
} else { | ||
hostURL = nil | ||
} | ||
|
||
return Credentials(accessToken: accessToken, host: hostURL) | ||
} | ||
} | ||
|
||
static var configuration = CommandConfiguration( | ||
commandName: "mapbox-directions-swift", | ||
abstract: "'mapbox-directions-swift' is a command line tool, designed to round-trip an arbitrary, JSON-formatted Directions or Map Matching API response through model objects and back to JSON.", | ||
subcommands: [Match.self, Route.self] | ||
) | ||
|
||
fileprivate static func validateInput(_ options: ProcessingOptions) throws { | ||
|
||
guard FileManager.default.fileExists(atPath: options.configPath) else { | ||
throw ValidationError("Options JSON file `\(options.configPath)` does not exist.") | ||
if !FileManager.default.fileExists(atPath: (options.config as NSString).expandingTildeInPath) && URL(string: options.config) == nil { | ||
throw ValidationError("Configuration is a nonexistent file or invalid request URL: \(options.config)") | ||
} | ||
} | ||
} | ||
|
@@ -54,7 +71,7 @@ extension Command { | |
} | ||
|
||
mutating func run() throws { | ||
try CodingOperation<MapMatchingResponse, MatchOptions>(options: options).execute() | ||
try CodingOperation<MapMatchingResponse, MatchOptions>(options: options, credentials: try credentials).execute() | ||
} | ||
} | ||
} | ||
|
@@ -72,7 +89,7 @@ extension Command { | |
} | ||
|
||
mutating func run() throws { | ||
try CodingOperation<RouteResponse, RouteOptions>(options: options).execute() | ||
try CodingOperation<RouteResponse, RouteOptions>(options: options, credentials: try credentials).execute() | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why
DirectionsOptions(url:)
has to returnnil
when given a URL whose path conflicts with theabridgedPath
property. If it doesn’t returnnil
, then either RouteOptions and MatchOptions would be equally likely to initialize based on the same URL, mooting the coalescing operator.Maybe it would be cleaner to nix the
OptionsType
generic parameter in favor of an enumeration with associated values that gets passed into this method, since the two APIs aren’t all that different. But I’ll leave it for now in case another API needs this kind of flexibility.