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

Enable macro usage for users to define colormaps #481

Merged
merged 7 commits into from
Feb 27, 2024

Conversation

jonaspleyer
Copy link
Contributor

This PR enables the user to leverage the macro define_linear_interpolation_color_map to easily create custom colormaps.

In addition, I have hidden the documentation of macros in the crate root and instead included them in the correct module.

Some functionalities which are required are hidden since they are not meant to be used by the end-user.

@jonaspleyer
Copy link
Contributor Author

jonaspleyer commented Jun 27, 2023

It seems that one of the checks was canceled. I cannot see any apparent reason as to why It could have happened.

@AaronErhardt
Copy link
Member

I've re-run the canceled check. I've no idea why it was canceled either.

Anyway, I wonder whether it makes sense to use #[doc(hidden)] followed by doc comments. AFAIK there will be no docs generated for hidden items so using doc comments on such items doesn't really make sense, or am I wrong? Can you maybe show how the docs look for you locally?

@jonaspleyer
Copy link
Contributor Author

The documentation is hidden on my machine. To me it makes sense if another developer is looking at your code and trying to understand it. Maybe I was too motivated in writing doc-tests. Since they present an actual computational overhead, it might be reasonable to omit them.

@jonaspleyer
Copy link
Contributor Author

To advance this PR: I moved the relevant docstrings to be inline comments only. Furthermore, I hid the count! macro since it is only being used as a helper macro and shortened the macro to define colormaps in order to be easier to use and not having to write such a long name.

Please let me know if this fixes your previously mentioned concern.

@AaronErhardt
Copy link
Member

The code looks good, but I think there are a couple of plots that should not be part of the commits.

- also hide the count! macro, since it is only used as a helper
@jonaspleyer
Copy link
Contributor Author

Sorry this was a total oversight on my part. I have rebased my master branch to not include the plots in the git history at all.

Copy link
Member

@AaronErhardt AaronErhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@AaronErhardt AaronErhardt merged commit bb25570 into plotters-rs:master Feb 27, 2024
18 checks passed
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