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

Remove @compact(name=...) and replace with NoShow #19

Merged
merged 9 commits into from
Oct 13, 2023
Merged

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Aug 29, 2023

As part of trying to simplify @compact, this removes the special keyword name, 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 trivial NoShow layer which does this.

NoShow needs tests.

Edit: Note that master is 0.2 since #16 but had not been released yet.

@MilesCranmer
Copy link
Contributor

Good idea. 👍 from me!

Copy link
Contributor

@gaurav-arya gaurav-arya left a 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!

src/compact.jl Show resolved Hide resolved
test/compact.jl Show resolved Hide resolved
test/compact.jl Outdated Show resolved Hide resolved

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
Copy link
Contributor

@gaurav-arya gaurav-arya Aug 30, 2023

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-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (cc0e36f) 79.63% compared to head (f9a28cd) 77.72%.
Report is 2 commits behind head on master.

❗ 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     
Files Coverage Δ
src/Fluxperimental.jl 100.00% <ø> (ø)
src/compact.jl 91.25% <100.00%> (-1.23%) ⬇️
src/noshow.jl 66.66% <66.66%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@gaurav-arya gaurav-arya left a 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?

@mcabbott
Copy link
Member Author

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.

mcabbott and others added 2 commits October 12, 2023 16:56
Co-authored-by: Gaurav Arya <gauravarya272@gmail.com>
Co-authored-by: Gaurav Arya <gauravarya272@gmail.com>
test/compact.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Member Author

mcabbott commented Oct 13, 2023

Maybe this is ready? Would like to rebase #20 on top...

@gaurav-arya
Copy link
Contributor

Sure, yes the current state looks good to me.

@mcabbott mcabbott merged commit 753127d into master Oct 13, 2023
3 checks passed
@mcabbott mcabbott deleted the compact_noname branch October 13, 2023 13:13
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.

4 participants