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

Add optional collection size indicator #296

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

alexkunin
Copy link
Contributor

This is my attempt to add requested functionality. Few notes:

  1. Disabled by default, can be enabled by setting FX_SHOWSIZES environment variable to a truthy value(true, on, 1, yes, case does not matter).
  2. Size indicator is formatted as /* 123 */ -- to avoid confusing with actual values (e.g. [ 123 ] could be read as array with single numeric element)
  3. Theme has separate color for size indicator, currently in all themes it is the same color which was used for ...
  4. For arrays, it replaces ... with size indicator.
  5. For objects, it replaces first key and ... with size indicator (notation like {"name":/* 123 */ would be very confusing).

closes #278

@antonmedv
Copy link
Owner

antonmedv commented Mar 15, 2024

Nice! But is it possible to display numbers outside brackets?

"foo": [...], // 123

@alexkunin alexkunin force-pushed the collection-size-indicator branch from cb151fe to 5cc0b5a Compare March 15, 2024 05:14
@alexkunin
Copy link
Contributor Author

alexkunin commented Mar 15, 2024

Sure, done: counts are now in outside object/array.

One things I have noticed, previous iteration of my PR failed due to missing package slices. Which I had fixed by making sure the code works with go 1.20, per go.mod. But, actual error message looked like this:

Error: theme.go:8:2: package slices is not in GOROOT (/opt/hostedtoolcache/go/1.18.10/x64/src/slices)

So, tests seem to be executed with go 1.18, which also can be seen in .github/workflows/test.yml. Not sure if this is a big deal tho, just FYI.

@alexkunin alexkunin force-pushed the collection-size-indicator branch from 5cc0b5a to e9b6728 Compare March 15, 2024 05:26
@antonmedv antonmedv merged commit 64eefcc into antonmedv:master Mar 15, 2024
2 checks passed
@antonmedv
Copy link
Owner

Renamed to FX_SHOW_SIZE as it already once was: #138 (comment)

@antonmedv
Copy link
Owner

What about this style?

image

@antonmedv
Copy link
Owner

And what about showing size for expanded nodes as well?

image

@alexkunin
Copy link
Contributor Author

I don't mind adding that, but honestly it rises questions:

  1. Switching styles between collapsed and uncollapsed states might be confusing -- I mean, they're like totally different.
  2. Having size next to collapsed node is totally fine. But what if huge node (hundreds? thousands?) is expanded:
    • should I add indicator at the start?
    • or at the end?
    • or both?
    • what about both start and end of the node are out of the view?
  3. Previously if you select and copy fully expanded JSON, result was a valid JSON, but with this change it's not anymore.

What would be the next smallest change to implement to avoid breaking things too much and still add some vallue, in your opinion?

@antonmedv
Copy link
Owner

  1. Lets just use grey style.
  2. Only at first bracket. Not if size is 1.
  3. Yank value will always copy correct JSON.

@alexkunin alexkunin deleted the collection-size-indicator branch March 15, 2024 12:58
@alexkunin
Copy link
Contributor Author

Oh, you did that already... I thought you were suggesting me doing it. Well done!

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

Successfully merging this pull request may close these issues.

Show number on items for collapsed node
2 participants