Remove @compact(name=...) and replace with NoShow #19
Conversation
|
Good idea. 👍 from me! |
gaurav-arya
left a comment
gaurav-arya
left a comment
There was a problem hiding this comment.
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.
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. |
gaurav-arya
left a comment
gaurav-arya
left a comment
There was a problem hiding this comment.
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. |
4 participants
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
showprints 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 trivialNoShowlayer which does this.NoShowneeds tests.Edit: Note that master is 0.2 since #16 but had not been released yet.