vtourraine / AcknowList

Acknowledgements screen displaying a list of licenses, for example from CocoaPods and Swift Package Manager dependencies.
MIT License
784 stars 59 forks source link

Add regex to filter out premature line breaks #43

Closed azhang66 closed 6 years ago

azhang66 commented 6 years ago

This prevents text alignment issues, most visible on iPads, as in #41.

This PR was originally submitted with some of our extraneous internal commits, which caused the CI build to fail. This re-submission should be correct 😄

vtourraine commented 6 years ago

Hi! Sorry for the late reply.

That’s an interesting issue you’re trying to fix. Like you said, it’s not really a bug, but most libraries licenses have these superfluous line breaks. I want to keep AcknowList simple, and let people customize it if they need to. I’m not fond of adding a (fairly) complex regex, but this is so common that it might be worth it.

So, I’ll merge this in, but first we need to add a couple of unit tests to check what this regex is doing. Feel free to update your pull request accordingly; otherwise I’ll update it myself when I get the time.

Thanks a lot!

azhang66 commented 6 years ago

@vtourraine done! I've added three commits testing the regex on five common licenses, using the pods TYPFontAwesome (SIL OFL 1.1), pop (BSD), Alamofire (MIT), Charts (Apache 2), and TPKeyboardAvoiding (zLib). The unit test will load the RegexTesting plist, load the manually edited "ground truth" strings file, and compares the two files for each pod.

One small issue that may be worth improving in the future would be the format for encoding the ground truth license strings, but for now I think it should be fine.

Basically the ground truth strings file is stored in the plain text format: /// TEST <#> /// <Pod Name> /// <License Text>, where we use /// as a component delimiter. There are probably better ways of storing this text, but for now I think it'll be fine due to the difficulty of properly storing line breaks in other formats.

vtourraine commented 6 years ago

Hello Albert, and thanks a lot for the update!

I love the approach of testing against the most-used licenses, and I really appreciate the comments you put in the code (although it’s a bit ironic that you manually line-wrap your comments 😉).

I just made a few adjustments:

What do you think?

azhang66 commented 6 years ago

Sounds good! I like your way of testing a lot more — it is a much cleaner approach!

Ready to merge?

vtourraine commented 6 years ago

👍 🚀