Add support for file URLs in MTLURLValueTransformerName #262

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

mrh-is commented Mar 7, 2014

MTLURLValueTransformerName currently returns nil if a file URL is passed in. This also allows for file URLs to be used.

Owner

robb commented Mar 7, 2014

Thanks

Can you add a test, too?

@robb robb commented on an outdated diff Mar 7, 2014

...SValueTransformer+MTLPredefinedTransformerAdditions.m
@@ -23,7 +23,7 @@ + (void)load {
MTLValueTransformer *URLValueTransformer = [MTLValueTransformer
reversibleTransformerWithForwardBlock:^ id (NSString *str) {
if (![str isKindOfClass:NSString.class]) return nil;
- return [NSURL URLWithString:str];
+ return ([NSURL URLWithString:str] ?: [NSURL fileURLWithPath:str]);

@robb robb commented on the diff Mar 10, 2014

MantleTests/MTLPredefinedTransformerAdditionsSpec.m
@@ -25,6 +25,11 @@
expect([transformer transformedValue:URLString]).to.equal([NSURL URLWithString:URLString]);
expect([transformer reverseTransformedValue:[NSURL URLWithString:URLString]]).to.equal(URLString);
+ // Write temporary file for use in testing file URLs
+ NSURL *fileURL = [NSURL fileURLWithPath:[NSTemporaryDirectory() stringByAppendingPathComponent:@"file.txt"]];
@robb

robb Mar 10, 2014

Owner

Let's use a fixed String and URL as input and not rely on the test environment's temp dir.

It also looks like there a spaces in the beginning of the line.

@mrh-is

mrh-is Mar 10, 2014

I was trying that, but if you don't give it a real path, +fileURLWithPath: gives you nil. Perhaps I just couldn't find the right way though.

Owner

jspahrsummers commented Mar 10, 2014

I'm not sold on this. If you're expecting a path, I think you should create a transformer that does this explicitly. Otherwise, invalid URLs could end up as paths on the user's filesystem.

Owner

robb commented Mar 10, 2014

That's a good point, especially when combined with implicit transformers

mrh-is commented Mar 10, 2014

Hmm, true @jspahrsummers. Probably a better idea. I shall do so forthwith. Thanks for the suggestion!

mrh-is closed this Mar 10, 2014

Owner

jspahrsummers commented Mar 13, 2014

No problem! Sorry for the terseness of my previous reply. I really appreciate you taking the time to open this as a pull request for discussion.

mrh-is commented Mar 14, 2014

No, I appreciate the feedback! I hadn't thought of those perspectives, and I'm glad to learn from you guys.

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