Skip to content

Add debug representation for MarkerTree#6129

Closed
ibraheemdev wants to merge 1 commit intomainfrom
ibraheem/marker-tree-debug
Closed

Add debug representation for MarkerTree#6129
ibraheemdev wants to merge 1 commit intomainfrom
ibraheem/marker-tree-debug

Conversation

@ibraheemdev
Copy link
Member

@ibraheemdev ibraheemdev commented Aug 15, 2024

Summary

Adds a simple Debug representation for MarkerTree based on the underlying decision diagram. For example, python_version == '3.7' and os_name == 'Linux') or os_name == 'Windows' is displayed as:

python_version<3.7 | >3.7 -> 
  os_name<Windows | >Windows -> false
  os_name==Windows -> true
python_version==3.7 -> 
  os_name<Linux | >Linux, <Windows | >Windows -> false
  os_name==Linux | ==Windows -> true

I'm not sure we want to use this form for our debug logs because it could be confusing to users but we've also run into issues where markers looked equal when simplified despite being represented slightly differently..

@ibraheemdev ibraheemdev added the preview Experimental behavior label Aug 15, 2024
@ibraheemdev ibraheemdev force-pushed the ibraheem/marker-tree-debug branch from f362838 to 555e808 Compare August 15, 2024 21:22
@ibraheemdev
Copy link
Member Author

We should probably add a separate method display_maybe_empty for user facing output.

@ibraheemdev ibraheemdev marked this pull request as draft August 15, 2024 21:29
@BurntSushi
Copy link
Member

but we've also run into issues where markers looked equal when simplified despite being represented slightly differently..

Can you show an example? I really like how our Debug implementation works today, especially since it fits on one line. (Although that does break down for larger markers.)

@zanieb zanieb removed the preview Experimental behavior label Aug 20, 2024
BurntSushi pushed a commit that referenced this pull request Sep 6, 2024
This PR revives #6129, but is less bold:

* It doesn't rename anything. (I think the rename is probably right
  though.)
* It doesn't change the _default_ `Debug` impl. Instead, it offers this
  as a new `MarkerTree::debug_graph` method.

I found this pretty useful for debugging since it gives a display format
that is more faithful to the internal representation of a `MarkerTree`.
So I think it's worth having around. But making it available in `Debug`
is perhaps a bridge too far since it isn't as familiar as the typical
PEP 508 representation and isn't as succinct.

I did consider printing this when using `{:#?}` (i.e., the "alternate"
debug representation), but too many things use that (like `insta` I
think) to make it practical.

Closes #6129
BurntSushi pushed a commit that referenced this pull request Sep 6, 2024
This PR revives #6129, but is less bold:

* It doesn't rename anything. (I think the rename is probably right
  though.)
* It doesn't change the _default_ `Debug` impl. Instead, it offers this
  as a new `MarkerTree::debug_graph` method.

I found this pretty useful for debugging since it gives a display format
that is more faithful to the internal representation of a `MarkerTree`.
So I think it's worth having around. But making it available in `Debug`
is perhaps a bridge too far since it isn't as familiar as the typical
PEP 508 representation and isn't as succinct.

I did consider printing this when using `{:#?}` (i.e., the "alternate"
debug representation), but too many things use that (like `insta` I
think) to make it practical.

Closes #6129
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.

3 participants