dogmatiq / dapper Goto Github PK

Type names are always omitted for the reflect.String and reflect.Bool kind, because the I considered these literals always to be unambiguous, however this is not true when the type is not the default string or bool, but rather some user-defined type.

The default way that the RWLock and Once are rendered does not offer much useful information. For example:

    once:             {
        done: 0
        m:    {
            state: 0
            sema:  0
        }
    }
    m:                {
        w:           {
            state: 0
            sema:  0
        }
        writerSem:   0
        readerSem:   0
        readerCount: 0
        readerWait:  0
    }

An improvement would be to simply render the types, or if possible showing the locked state (for locks) or done state (for once) in a more friendly manner.

This should render as much like a regular typed map as possible without being ambiguous.

See dogmatiq/dogma#112 and dogmatiq/dogma@b3c4407 for an example.

• time.Time
• time.Duration

They are currently indented to align with the widest line of any map key, like so:

*engine.Engine{
    roles:  {
        {
            id:   17776544
            name: "messages.OpenAccount"
        }:                                               "command"
        {
            id:   17800064
            name: "messages.AccountCreditedForDeposit"
        }:                                               "event"
}

Note that I'm talking specifically about the alignment of the words "command" and "event" in this example.

I question the readability of this formatting, and further more it's probably less deterministic, since the values or presence/absence of other map keys affects how such keys are formatted. This is true even for single-line map keys.

I see a few options:

1. Don't align values if their key is multi-line -- this is still non-deterministic for single-line keys, but should improve readability overall
2. Don't align any values -- this is the most deterministic, but readability will be sacrificed in the simple case
3. Render keys and values on different lines -- this would remove all alignment, maintaining determinism, we'd have to come up with a readable way to disambiguate the keys/values.

This can be done incrementally, rather than trying to port them all at once.

It's annoying seeing interleave output in concurrent programs and Print() has essentially become a debug tool.

For example, if the key type is numeric use a numeric sort.

Especially with the introduction of Go modules, it's common to have two types with the same name and package name.

Showing fully qualified type names would satisfy Dapper's design goal of being unambiguous. (at the cost of being more verbose).

In reviewing PR #27, which moves the recursion detection logic, I noticed this paragraph in the documentation of reflect.Type.Pointer():

If v's Kind is Slice, the returned pointer is to the first element of the slice. If the slice is nil the returned value is 0. If the slice is empty but non-nil the return value is non-zero.

I'm not sure about that "first element of the slice" thing, it seems like that might incorrectly render the recursion markers at the elements of the slice, rather than at the slice itself. We probably just want to make sure there is a test for this case.

This would produce more concise output. For maximum determinism the field alignment would still need occur at the width of the longest field name, even if it was omitted.

*queue.Queue{
    DataStore:  *fixtures.DataStoreStub{
        DataStore:                *memory.dataStore{
            db:     {
                RWMutex: {
                    m:        <unlocked>
                    readers:  0
                    unlocked: 0xc0002d4f00 1/1
                    retry:    nil
                }
                open:    1
                events:  nil
                queue:   {

Notice the indent of *memory.dataStore on the third line. I'll have to work on a better reproduction for this, but just creating the issue before I forget.

Ideally this should render the number of items in the pool, but I'm not sure how simple this is.

/cc @danilvpetrov

Type names are not shown when the reflect.Type value is obtained from an unexported struct field.

For example, reflect.Type renders something like:

*reflect.rtype{
    size:       0x38
    ptrdata:    0x28
    hash:       2359684846
    tflag:      7
    align:      8
    fieldAlign: 8
    kind:       25
    alg:        {
        hash:  0x10c5da0
        equal: 0x10c5e60
    }
    gcdata:     21
    str:        35705
    ptrToThis:  61472
}

But it'd be much more useful to see something like reflect.Type("<name>").

Ideally this should render the pending count.

This could use some kind of hexdump style rendering, like byte-slices. In fact, they might already render somewhat reasonably as of v0.3.4, I haven't tried.

As it stands, each filter and visitor method must call unsafereflect.MakeMutable() manually. As this is an internal package, third-party filters would not be able to call it at all.

Rather than exposing MakeMutable() as part of Dapper's public API, inside mustVisit() we could replace v.Value with the result of MakeMutable(v.Value).

This means that all filters and visitor methods would automatically receive a mutable value and that any immutable reflect.Value provided to a filter's f() format function would also be made mutable.

I'm pretty sure that should totally negate the need for any additional calls to MakeMutable().