search

Dimillian / RedditOS

The product name is Curiosity, a SwiftUI Reddit client for macOS Big Sur
Apache License 2.0
3.95k stars 199 forks source link

Introduce url extension to help avoid force unwrapping for static urls and add unit test for changed model #24

Closed DanKorkelia closed 3 years ago

DanKorkelia commented 3 years ago

Hi 👋 Happy new year.

Wanted to provide a tiny contribution to your excellent project wit this little addition to help avoid force unwrapping static URLs. Also added a unit test for model. Not sure what the exact policy is but thought it might be helpful.

Best wishes Dan

ericlewis commented 3 years ago

One note though: I would prefer the arg be named staticString instead as that clarifies what is going on.

URL(staticString: "")

DanKorkelia commented 3 years ago

Hey! Happy to chip in. Glad you liked the change. Think I might have lifted this from Paul H actually but I do read John's blog on regular basis too :) I'll update the argument no problem.

ericlewis commented 3 years ago

I meant the extension should be used like URL(staticString: "") but you're still anonymizing the actual argument. Delete the _ and update the call sites :)

My reasoning for this is because we don't have to know about the extension- we can read in the call sites to understand why URL is behaving differently instead of tracking back to the extension. I think when extending Foundation or any 1st party types it's always important to be clear what is going on when it's used places too.

DanKorkelia commented 3 years ago

That's a good argument for more clarity I'm happy with that. I slightly rushed to change the name and only realised you were after explicate name at call site. I will update.