Skip to content

Unneeded argument in get_linestyle #3355

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

Closed
wants to merge 3 commits into from

Conversation

JamesRamm
Copy link

Removed unnecessary argument style in get_linestyle of
GraphicsContextBase.

Also made GraphicsContextBase inherit from object to support any
future propertyfication...

Removed unnecessary argument `style` in `get_linestyle` of
`GraphicsContextBase`.

Also made `GraphicsContextBase` inherit from object to support any
future propertyfication...
@tacaswell tacaswell added this to the v1.5.x milestone Aug 9, 2014
@tacaswell
Copy link
Member

This is a backwards incompatible change and I am a bit distressed that none of our tests are failing.

This needs an entry in api/api_changes. See #3349 for how we are going to start doing this.

@tacaswell
Copy link
Member

This closes #3354

@WeatherGod
Copy link
Member

It is technically a backwards incompatible change, but it seems that
nobody has been using this particular method. I did a grep through our
codebase and did not find an instance of this method being used.
GraphicsContexts don't get directly exercised (probably because they aren't
documented very well). I am a bit concerned about why this particular
getter method was never used. Does this indicate a design flaw in our
graphics stack? Are there other getters that are not used? These are all
important questions.

But, yes, at the very least, let's note it in the api_changes for
completeness. Just because someone was sloppy in the past does not mean we
should be sloppy now.

On Sat, Aug 9, 2014 at 9:08 AM, Thomas A Caswell notifications@github.com
wrote:

This closes #3355 #3355


Reply to this email directly or view it on GitHub
#3355 (comment)
.

@tacaswell
Copy link
Member

I am 👍 on this change, just want to make sure it gets documented correctly.

@JamesRamm
Copy link
Author

In most cases in the renderer draw_ methods, the GraphicsContext is just passed straight through to the C++ renderer functions.... I don't know if the get_ methods are required for this ?

In the python code, it mostly just requires the set_ methods; the GraphicsContext objects only seem to be created in draw methods, where the artist properties are transferred across...so no 'getting' required.

I would imagine you would never really see any of the GraphicsContext get methods being used (at least in python code)...
But, with them there, it makes it easy to make properties:

property(get_linestyle, set_linestyle, set_linestyle.__doc__)

Fixes issue #1 and issue #2
Added a style property to artist and created a Style class in style.py.
This requried renaming existing style subpackage to old_style
Made dashes and sketch_params into properties in the style class. Fixes
issue #2
@JamesRamm
Copy link
Author

Just checking back in on this. Is there something I need to do?

@WeatherGod
Copy link
Member

It does seem to need a rebase, or something. Why is the style.py file being added here?

@tacaswell
Copy link
Member

moving the style module is a non-starter.

@JamesRamm
Copy link
Author

Hmm...The commit I intended to pull was bd06d88, which should only have 1 changed file..

In fact, this is the original pull (see top of page).

the more recent commits shouldn't be linked to this pull request.

@tacaswell
Copy link
Member

Closing this as moving the style module is a non-starter.

@JamesRamm If you are still interested in working on this have a look at #3424 and the discussion around MEP25.

@tacaswell tacaswell closed this Oct 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants