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

ENH: public API render methods to PlotContainer and FixedResolutionBuffer #4121

Merged

Conversation

neutrinoceros
Copy link
Member

PR Summary

A simple addition to the public interface would be useful in #4118 to avoid using private methods directly.
Content:

  • ENH: add a PlotContainer.render method (public interface for _setup_plots)
  • DOC: avoid using private _setup_plots method in user docs

For backward compatibility, I avoided simply renaming _setup_plots.
Hopefully the intented usage is clear from the docstring and the docs update.

I'm not completely certain that my use of the @validate_plot is warranted or justified, and I would particularly appreciate feedback specifically on this.

@neutrinoceros neutrinoceros added enhancement Making something better docs labels Sep 13, 2022
@neutrinoceros
Copy link
Member Author

just noticed that FixedResolutionBuffer could also use a render method to clarify usage of fib[item], I'll include a patch for this too

@neutrinoceros neutrinoceros marked this pull request as draft September 13, 2022 16:38
@neutrinoceros neutrinoceros changed the title ENH: add a PlotContainer.render method (public interface for _setup_plots) ENH: public API render methods to PlotContainer and FixedResolutionBuffer Sep 13, 2022
@neutrinoceros neutrinoceros changed the title ENH: public API render methods to PlotContainer and FixedResolutionBuffer ENH: public API render methods to PlotContainer and FixedResolutionBuffer Sep 13, 2022
@neutrinoceros neutrinoceros marked this pull request as ready for review September 13, 2022 17:31
@matthewturk
Copy link
Member

I like the idea, but I'm not sure why we'd want to add @validate_plot here if it's not required by _setup_plots.

@neutrinoceros
Copy link
Member Author

yup I actually don't think it's needed.

@neutrinoceros neutrinoceros marked this pull request as draft September 21, 2022 18:50
@neutrinoceros neutrinoceros force-pushed the plotcontainer_render_method branch from ce5a58e to a27407e Compare September 21, 2022 18:51
@neutrinoceros neutrinoceros marked this pull request as ready for review September 22, 2022 06:46
@neutrinoceros
Copy link
Member Author

@matthewturk I removed the unnecessary decorator, this is now ready for review again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs enhancement Making something better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants