Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time string not always displayed #3342

Closed
sylr opened this issue Apr 25, 2023 · 8 comments · Fixed by #3349
Closed

Time string not always displayed #3342

sylr opened this issue Apr 25, 2023 · 8 comments · Fixed by #3349

Comments

@sylr
Copy link
Contributor

sylr commented Apr 25, 2023

  1. What version of Delve are you using (dlv version)?
dlv version
Delve Debugger
Version: 1.20.2
Build: $Id: e0c278ad8e0126a312b553b8e171e81bcbd37f60 $
  1. What version of Go are you using? (go version)?
go version go1.20.3 darwin/arm64
  1. What operating system and processor architecture are you using?
    Darwin
  2. What did you do?
package main

import (
	"fmt"
	"time"
)

type Interval struct {
	Start time.Time
	End   time.Time
}

func main() {
	t := time.Now().Truncate(time.Second)
	i := Interval{Start: t, End: t.Add(time.Hour * 24)}

	ts := make([]Interval, 0, 1)
	ts = append(ts, i)

	fmt.Printf("ts: %v\n", ts)
}
  1. What did you expect to see?

I'd like to see the Time string in ts[0].Start and ts[0].End.

  1. What did you see instead?

image

@alexsaezm
Copy link
Contributor

For what it's worth, this also happens on linux/amd64. I'm wondering what should be the expected output, exactly like i.Start?

@sylr
Copy link
Contributor Author

sylr commented Apr 25, 2023

For what it's worth, this also happens on linux/amd64. I'm wondering what should be the expected output, exactly like i.Start?

That would be my expectation yes.

@sylr
Copy link
Contributor Author

sylr commented Apr 25, 2023

Also, is that normal that ts[0].Start and ts[0].End are displayed as *time.Time instead of time.Time ?

@aarzilli
Copy link
Member

I think because of the nesting depth those time.Time value don't get loaded immediately and therefore don't get the formatting.

@alexsaezm
Copy link
Contributor

For the record, looks like dap can be configured so it might interested to add a configuration option like "depth" and a sane default.

If the solution sounds ok, I'll work on a PR.

@derekparker
Copy link
Member

I'm not sure if the DAP folks actually intend for it to be configurable given https://github.com/go-delve/delve/blob/master/service/dap/server.go#L256. We could experiment with increasing the depth to 2, or maybe this is something better solved on the VSCode side?

cc @hyangah

@suzmue
Copy link
Contributor

suzmue commented Apr 28, 2023

Looks like this is actually just a bug in server/dap, the variable is loaded correctly but the variable with the loaded information isn't used to format the output. Will send a PR to fix.

suzmue added a commit to suzmue/delve that referenced this issue Apr 28, 2023
The only part of the reloaded value that was used was the
Children. This caused a bug where the Time format string was
reloaded, but it was not being displayed.

This is a very minimal change to get this to work. We should
probably also do the TODO at the top of the section.

Fixes go-delve#3342
suzmue added a commit to suzmue/delve that referenced this issue Apr 28, 2023
The only part of the reloaded value that was used was the
Children. This caused a bug where the Time format string was
reloaded, but it was not being displayed.

Fixes go-delve#3342
@suzmue
Copy link
Contributor

suzmue commented Apr 28, 2023

Sent a PR for fixing the bug where the time.Time arguments have the values.

Screenshot 2023-04-28 at 3 19 16 PM

We also need to address the inconsistency with Interval sometimes having the children loaded and displaying the formatted string in its value and sometimes not. (i displays time.Time but ts[0] doesn't).

I think ideally we will be able to avoid making the depth configurable, so maybe trying to make the depth 2 could be a good way to resolve it. We may be able to be selective about where we increase the depth as well.

aarzilli pushed a commit that referenced this issue Apr 29, 2023
The only part of the reloaded value that was used was the
Children. This caused a bug where the Time format string was
reloaded, but it was not being displayed.

Fixes #3342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants