Parse localizable strings comments

[nolanw]

This is an attempt to start implementing #232.

• Switch to a regex for parsing .strings files so we can capture comment.
• Warn when a duplicate key is encountered in a .strings file (previously the last value was silently chosen the winner).
• Include comment that precedes .strings file entry, if present, in R.generated.swift.

I'm a bit stuck on the last part, where we carry the parsed strings comment all the way through to R.generated.swift. I'd appreciate some guidance! (Or if someone wants to just take it from here, go for it!)

[tomlokhorst]

Very cool! Thanks for this PR!

I quickly scrolled through this, I'll look in to it more deeply when I have some more time.

The warnings about duplicate identifiers should be part of the code generation step, not the parsing step. That's because multiple files can be merged together at code generation time. When parsing it is not yet known which keys will become duplicate unique identifiers.

In this case that means the parseString function should actually return a list of tuples (or structs) instead of the dictionary it returns now.

As for the rest of it; I'll take a closer look later, or maybe someone else will before that.

[nolanw]

The warning I added is meant for duplicate keys in a single file; I thought there was a todo somewhere about maybe adding a warning for that. NSPropertyListSerialization just silently takes the last of the duplicates.

edit: this was a bit snarky, sorry. If that's the wrong place for the warning then I can move it no problem. But I'm not sure we're thinking of the same warning?

[tomlokhorst]

Standard R.swift behaviour for duplicate keys (single file or not), is to skip generating code for that key and only print a warning.

An extra feature would be to extend that behaviour to strings files. That means collecting all keys (including duplicates within a single file), and have the code generator print the warnings (which it already does).

This also allows for extensibility in the future, where some other process between the parser and the code generator does something with all the keys. Like show them to the user or filter them based on some heuristic.

Again, this is an additional feature that R.swift currently doesn't have. But the reason for building a custom parser, instead of using the plist parser, is to enable additional features.

[nolanw]

Here's a first attempt at the rest of this PR, getting the parsed .strings file entry comment into the generated documentation comment. No idea if this is the right way to do this, so feedback very welcome!

I removed the warning emitted during parsing (thanks @tomlokhorst for patiently explaining). We can add it to the appropriate place sometime later. Here's the todo I was talking about that mentions this warning, if anyone's curious (shout out to fellow Nolan creating that file).

I've also modified the regex a bit to only consider a comment directly preceding (i.e. up to 1 line away from) a .strings file entry as associated with that entry. Previously the comment atop the file was getting attached to the first entry, and that's not helpful. Unfortunately (?) I couldn't figure out how to do this entirely with the regex.

[nolanw]

It's been a month, is there anything I can do to move this PR along? Y'all won't hurt my feelings if you share what you don't like about it!

[tomlokhorst]

Hey Nolan, sorry for leaving this PR hanging for over a month...

Due to both me and @mac-cain13 being busy with other things, R.swift maintenance is on a bit of a summer break at the moment.

Unfortunately that means issues and pull requests start stacking up. Sorry about that.

The next thing I plan on doing for this PR is;

• Test this branch agains my internal apps using R.string.*, to see if anything breaks.

Hopefully I'll find the time to do that in the next week or so.

[nolanw]

No worries, enjoy the break!

[tomlokhorst]

One thing that would be nice to add to this PR: Tests :-)

• In ResourceApp add a comments.strings which has different keys with comments
• In ResourceAppTests/StringTests add XCTAssertEqual to check if the comments are correctly parsed

[tomlokhorst]

I just tested this on my biggest project that uses R.string.*.
The parser works great! It didn't skip anything, I did find an accidental duplicate key in my project.

I think these should be the test cases:

/* comment doesn't belong to any key */

"key1" = "One";

/* comment belongs to key2 */
"key2" = "Two";

"key3" = "Three"; /* comment belongs to key3 */
"key4" = "Four";

/* first part of comment for key5 */
"key5" = "Five"; /* second part of comment, should be merged */

/* big, multiline, comment
 * belonging to key6
 */
"key6" = "Six";

Also nice to have are single line comments:

// comment doesn't belong to any key

"key1" = "One";

// comment belongs to key2
"key2" = "Two";

"key3" = "Three"; // comment belongs to key3
"key4" = "Four";

// first part of comment for key5
"key5" = "Five"; // second part of comment, should be merged

// big, multiline, comment
// belonging to key6
"key6" = "Six";

[nolanw]

Tests sound good! I have a question about how to implement them. Does it make sense to include a comment property on StringResource? I don't anticipate these comments being useful to have in code, I had expected them just to be in the Quick Help. However that makes them more difficult to test. Any suggestions for how to test the generated comments?

[tomlokhorst]

Yes, I do think it makes sense to add a comment property.

The StringResource exists to expose all values that R.swift has collected to third party libraries to build against. I don't think there's a reason to exclude the comments from that.

Indeed, the tests can also use the comment: String? property.

[nolanw]

Cool, that makes sense. I've added the comment property in my fork of R.swift.Library and updated the submodule here; I'll open a PR over there once this one's in good shape.

Thanks for so patiently explaining like every part of this project to me as I go.

I started messing with the regex to consider //-comments and both leading and trailing comments. I got pretty lost. I tried again with an NSScanner approach and despite the inflation in lines of code it seems clearer to me. There are currently failing strings tests but they're cosmetic; I need to nail down what the merged comments should look like and then test that they look like that. I'll look into the other test failures too, but that's all for today.

[tomlokhorst]

The comment property is now available in the StringResource in the master branch of R.swift.Library.

[tomlokhorst]

I've created a Swift 3 version of this PR: #283