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 applicable check to @show_special #1595

Closed

Conversation

lgoettgens
Copy link
Collaborator

Follow-up to #1583 by @joschmitt.

This contains the changes from #1594 to avoid conflicts. The changes of this PR are solely contained in the last commit.

This change enables us to use the @show_special macro with both signatures (with/without MIME type) extensively without the need to make sure that both dispatches exist for all show_* functions. The macro now only does something if the custom show function matches the signature of the macro.

(I cannot choose @joschmitt as a reviewer, but please have a look as well.)

@joschmitt
Copy link
Collaborator

Hm, I understand that at the moment it is a lot easier (and probably even necessary) to have this because not all show variants support the MIME"text/plain" stuff.
But in the long run, I think I would prefer to get an error if the method is missing. The printing system is already hard enough to debug with all the macros and io-keywords and so on.

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

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

Comparison is base (6fda78f) 87.17% compared to head (e4c8f1f) 85.10%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/PrettyPrinting.jl 17.74% 51 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1595      +/-   ##
==========================================
- Coverage   87.17%   85.10%   -2.07%     
==========================================
  Files         115      115              
  Lines       29565    29560       -5     
==========================================
- Hits        25774    25158     -616     
- Misses       3791     4402     +611     

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

@lgoettgens lgoettgens marked this pull request as draft February 5, 2024 14:11
@lgoettgens
Copy link
Collaborator Author

You are right. I should rather change the way I call the macro instead of changing the macro

@lgoettgens lgoettgens closed this Feb 5, 2024
@lgoettgens lgoettgens deleted the lg/show-special-applicable branch February 5, 2024 15:12
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.

2 participants