theappbusiness / ConfigGenerator

Configuration file code generator for use in Xcode projects
MIT License
157 stars 19 forks source link

Fix when using objc = true with optional value will generate swift code. #23

Closed yliu342 closed 7 years ago

samdods commented 7 years ago

hey YK, great to see this PR... but have you considered throwing an error as soon as a ? is encountered in the mapping file?

all of this additional logic would then be redundant.

the user of this tool specify a ? in the mapping file if they are using it for an Objective-C project, so they should be alerted to their mistake.

samdods commented 7 years ago

hey @yliu342, can you describe an example use case for this please?

yliu342 commented 7 years ago

Hey @samdods think of a case where you have a code that take the value in configen and append some extra text after it. But for one environment, you don't need that value. If you have an empty string returned in configen @"" then the code will append some extra text after it which will break your code. But if you return nil for it then it won't even append the extra string.

I don't know if that makes sense to you. : )

samdods commented 7 years ago

@yliu342, do you have a real life example? i'm just curious as to why this support is being added - is it needed for a real life use case?

the reason i'm uneasy about this is that optionals are treated as custom types... they're barely* supported by Configen for Swift, so i don't see why we should support them for Objective C.

if you declare your type in the mapping file as String, then you provide the value in the configuration plist as, say, some value. whereas if you declare the type as String?, you need to provide the value with @ symbol and quotation marks, i.e. you have to provide the value in your plist as @"some value". that's the only reason setting it to nil works, because Configen doesn't add the @ symbol and quotation marks for you, for custom types (optionals included).

* support for optionals is a side effect of supporting custom types, see Custom Types
yliu342 commented 7 years ago

@samdods I agree with you on

optionals are treated as custom types... they're barely* supported by Configen for Swift

I don't have a real life example as when this is necessary but I can explain my thoughts. The pull request was opened while I was working on migrate GoAhead to use Configen. One of the target does not have value for an apiKey field and all other targets will append that apiKey field to some string. In this case, having an empty string @"" doesn't hurt. Then I thought about what if in a situation where you need to use the value get from Configen and append something after it, then @"" will make a difference. For example, you have a baseUrl property in configen and you code append some path or query after it. Then @"" will still append the path or query and when execute it may gives you error whereas nil in this case is safer.

I'm not sure if you agree with me on this but that was my thoughts while I opened this pull request. But I do agree with you this may well be a very edge case and Custom type are barely support in Swift and not support in Objective C and optional support is something that comes for free when support custom type.

samdods commented 7 years ago

thanks for explaining @yliu342 😄 but i think it's too much of an edge case to warrant the additional code complexity.

yliu342 commented 7 years ago

@samdods Yea, I kind of agree with you that this is an edge case and I'm happy to close this pull request

yliu342 commented 7 years ago

As discussed it may not be worth it to add the extra complexity to the code to address this edge case.