Skip to content

Added custom path ability#188

Merged
marmelroy merged 2 commits intomarmelroy:masterfrom
Calthings:custom-bundle-path
Sep 30, 2018
Merged

Added custom path ability#188
marmelroy merged 2 commits intomarmelroy:masterfrom
Calthings:custom-bundle-path

Conversation

@ashleybrgr
Copy link
Copy Markdown
Contributor

Allows the option to specify a custom bundle path as per git issue #131. Supports dependencies using Swift Package Manager since the package manager does not have the ability to copy over dependency resources.

@efirestone
Copy link
Copy Markdown

efirestone commented Feb 6, 2018

I'm not a maintainer of the project, but the diff looks good to me (and we're interested in getting this change as well).

A possible couple improvements for consideration:

  • Define JSONDataCallback as () throws -> Data rather than () -> Data?, with the thought being that a failure to get the data is really an error (there is no "successful" case where no data is returned). This is mostly just an API clarification.
  • Rather than defining MetadataManager.init as:
public init (JSONDataCallback: JSONDataCallback? = nil)

instead define it as

public init (JSONDataCallback: JSONDataCallback = defaultDataCallback)

where defaultDataCallback does the standard behavior of looking up the data from the local bundle. This has two small advantages:

  • The API is clearer. There is no guessing what nil means in the API.
  • There is no internal dependency on the local bundle's file. defaultDataCallback would be a static function, so after initialization the class has zero notion of the default bundle and its resources.

@efirestone
Copy link
Copy Markdown

@marmelroy, any chance of landing this soon? We'd love to be able to adopt this project but require this addition.

Comment thread PhoneNumberKit/MetadataManager.swift Outdated
*/
public init () {
territories = populateTerritories()
public init (metadataPath: String? = nil) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I recommend that the metadataPath is a file URL not a String. A String can be anything. You can validate that the URL is a fileURL. The code doesn't change much but using a URL here would make the intention more clear and probably reduce some user bugs.

@marmelroy
Copy link
Copy Markdown
Owner

Thanks all and sorry for the delay with this. I think this is a great improvement and will add it to the next release.

@marmelroy marmelroy merged commit 4732ebd into marmelroy:master Sep 30, 2018
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.

4 participants