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

Consider making marker private and instead only set it in add_markers or start_marking #141

Open
eteq opened this issue Jul 2, 2021 · 6 comments
Labels
API Issues that relate to the API itself rather than implementations enhancement New feature or request question Further information is requested

Comments

@eteq
Copy link
Member

eteq commented Jul 2, 2021

Right now there's a mix of a stateful and a non-stateful idiom in how you set the style of markers. self.markers is settable by the user, but it can also be set in start_marking. add_markers also uses self.markers, but does not have a markers keyword.

So I see two ways to fix this:

  1. Add a markers keyword to add_markers
  2. Do 1 and also make self.markers private
  3. remove markers from start_marking

I really don't like 3 because I think it just makes sense to be able to set your marker style when you add the marks. So that leaves 1 and 2 - 2 is more "pure" (i.e., there's then one way to do it), but has the downside of breaking backwards compatibility (which we may or may not care about).

Triggered by spacetelescope/jdaviz#699

cc @pllim @mwcraig

@eteq eteq added enhancement New feature or request question Further information is requested labels Jul 2, 2021
@mwcraig
Copy link
Member

mwcraig commented Jul 2, 2021

I assume you mean marker not markers in most of this?

@pllim
Copy link
Member

pllim commented Jul 2, 2021

Yes, marker (no "s").

@pllim
Copy link
Member

pllim commented Jul 2, 2021

@property
def marker(self):

def start_marking(self, marker_name=None,
marker=None):

@mwcraig
Copy link
Member

mwcraig commented Jul 2, 2021

The only argument that I can see against removing the markers attribute is that it may sometimes be handy to use the same style markers for two different things, in which case would be helpful to be able to get the current marker in some way. That could be done by making it a read-only property or by adding a get_current_marker_style() method.

@pllim

This comment has been minimized.

@pllim pllim added the API Issues that relate to the API itself rather than implementations label Jun 16, 2023
@pllim
Copy link
Member

pllim commented Jun 28, 2023

I don't have strong opinions on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Issues that relate to the API itself rather than implementations enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants