-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Remove @compact(name=...)
and replace with NoShow
#19
Conversation
Good idea. 👍 from me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a fan of the idea!
|
||
By default it prints `NoShow(...)` instead of the given layer. | ||
If you provide a string, it prints that instead -- it can be anything, | ||
but it may make sense to print the name of a function which will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a callable function to reconstruct the layer is what's desired, I thought a bit about allowing the user to specify the function (and its args) and incorporating something like https://github.com/JuliaLang/julia/blob/197180d8589ad14fc4bc4c23782b76739c4ec5a4/base/show.jl#L522 to make this more robust. I don't think it is worth the implementation complexity, and could also easily be added later if we really wanted it, so just noting for posterity.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #19 +/- ##
==========================================
- Coverage 79.63% 77.72% -1.92%
==========================================
Files 6 7 +1
Lines 221 220 -1
==========================================
- Hits 176 171 -5
- Misses 45 49 +4
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks fine to me. I don't spot a test that actually checks whether NoShow
works with a custom string, if there indeed isn't one could that be added?
I agree it's not super-well tested, but custom strings are almost too simple to go wrong. After f9a28cd the printing doesn't depend on this, only construction 1-arg method. |
Co-authored-by: Gaurav Arya <gauravarya272@gmail.com>
Co-authored-by: Gaurav Arya <gauravarya272@gmail.com>
Maybe this is ready? Would like to rebase #20 on top... |
Sure, yes the current state looks good to me. |
As part of trying to simplify
@compact
, this removes the special keywordname
, which replaced all printing with a given string. (This probably needs to be part of a breaking change.)The problem that solved was that sometimes the default
show
prints a lot. That's not really specific to@compact
. So perhaps it can be more cleanly addressed by making something orthogonal... so the second commit makes a trivialNoShow
layer which does this.NoShow
needs tests.Edit: Note that master is 0.2 since #16 but had not been released yet.