slashmo / gsoc-swift-baggage-context

BaggageContext library, extracted from slashmo/gsoc-swift-tracing
https://github.com/slashmo/gsoc-swift-tracing
Apache License 2.0
4 stars 2 forks source link

Semantically helpfully named empty baggage factory functions #26 #33

Closed ktoso closed 4 years ago

ktoso commented 4 years ago

Resolves #26 - Semantically helpfully named empty baggage factory functions

I went with:

ktoso commented 4 years ago

WDYT, @lukasa - opinions on keeping init() public so there'd be 3 ways of making a context -- empty, init, TODO() or should we drop the empty? I kind of felt the empty makes it more explicit in the code which is helpful;

Go call it "background" explicitly which is useful since it explains "well, you're a background task, you clearly dont have a context so just make that background one" which is also useful.

I'd like to stick to 2 ways to create one if possible, open to opinions.

Lukasa commented 4 years ago

I think having a .empty is good. Normally I’m opposed to that pattern over just having a zero-argument initializer, but in this case I actually think the reason I don’t like it is why it’s good. You should have to think about where you’re gonna get a context from, and explicitly ask for a new one if you don’t have one. I think background is an even better name, but I’m not so worried about that right now.

ktoso commented 4 years ago

You should have to think about where you’re gonna get a context from, and explicitly ask for a new one if you don’t have one.

Yeah exactly, this is to make this intentionally stand out and bit a bit ugly, and the TODO even being documenting why you're not doing the right thing etc.

I'm also liking background to be honest, but wonder if people would be ok with it. The goal is exactly that: to make it "weird and looks wrong" in normal apps - because they should not create those but keep passing along, so the background seems just weird enough to raise questions. The empty can still fly under the radar, but at least is explicit.

Let's ping some more folks: @tanner0101 @slashmo @adam-fowler @pokryfka @tachyonics

Options are IMO:

a)

b)

Please see https://golang.org/pkg/context/ as well as the docs in this PR for rationale. Thanks for chiming in 👍

slashmo commented 4 years ago

The goal is exactly that: to make it "weird and looks wrong" in normal apps - because they should not create those but keep passing along, so the background seems just weird enough to raise questions. @ktoso

Because of what you describe here, I prefer to go with .background & .TODO. .empty, in my opinion, is too easy to overlook, but if one sees .background they might be more inclined to think about why it's there.

ktoso commented 4 years ago

I made TODO less annoying by not requiring a reason, static analysis or crashing may be good enough for many users.