Add public debuggingName to WorkflowAction, use it in default toString()

[steve-the-edwards]

This is a more useful default implementation and allows clearer overriding via debuggingName.

[rjrjr]

This will also block data class WorkflowActions.

Block them, or just prevent them from generating horrible toString() implementations?

[rjrjr]

If we're making name a first class public val, isn't the final toString() business heavy handed? We can just make sure our logging always uses name. And we won't have to interfere with use of data class.

[steve-the-edwards]

we could, but we don't have control over that. if someone makes a massive data class MyAction: WorkflowAction with a tonne of members (as we have seen) then toString() will still print all those out.

final toString() prevents that and make sure that WorkflowAction logging is always based on the name which can only be so big (in reality)

[rjrjr]

Right, but I'm saying the real problem is that we're using toString() in logging at all, which we do because we had no alternative. But now name exists and we can log that — avoiding any call to toString() in production code as nature intended. That seems like a complete solution, I don't see why we need to mess with toString() as well.

[steve-the-edwards]

Right, but I'm saying the real problem is that we're using toString() in logging at all, which we do because we had no alternative. But now name exists and we can log that — avoiding any call to toString() in production code as nature intended. That seems like a complete solution, I don't see why we need to mess with toString() as well.

Ahhh IC. that makes a tonne of sense

[steve-the-edwards]

This will also block data class WorkflowActions.

Block them, or just prevent them from generating horrible toString() implementations?

I don't think there is a way that I've seen to subclass with a data class if toString() is final as you can't opt-out of the toString() implementation in the data class.

[rjrjr]

This will also block data class WorkflowActions.

Block them, or just prevent them from generating horrible toString() implementations?

I don't think there is a way that I've seen to subclass with a data class if toString() is final as you can't opt-out of the toString() implementation in the data class.

This compiles:

abstract class Foo {
  final override fun toString(): String {
    return super.toString()
  }
}

data class Bar(val s: String) : Foo() {

}

But I still think taking over toString() is a bridge too far.

[steve-the-edwards]

This will also block data class WorkflowActions.

Block them, or just prevent them from generating horrible toString() implementations?

I don't think there is a way that I've seen to subclass with a data class if toString() is final as you can't opt-out of the toString() implementation in the data class.

This compiles:

abstract class Foo {
  final override fun toString(): String {
    return super.toString()
  }
}

data class Bar(val s: String) : Foo() {

}

But I still think taking over toString() is a bridge too far.

Nice! My assumption was wrong then. This is even better. let's talk offline about why you don't like final toString()

[rjrjr]

Should update the @param for name to mention debuggingName instead of toString(). Just to draw people's attention to the better thing.

[steve-the-edwards]

Done.

[rjrjr]

Suggested change

	// Debugging name, will be returned by `toString()`
	/** Debugging name. Handy for logging and returned by `toString()`. */

[steve-the-edwards]

Done.

[rjrjr]

Not sure where toString() is leaking in, but can you instead change this test to rely on debugginName directly and avoid the need for this function? Same for the other instances.

[steve-the-edwards]

This one is a good candidate for that, but not the TracingWorkflowInterceptorTest as we want that to use the actual toString() or the traces won't be as useful.