Skip to content
This repository has been archived by the owner on Feb 9, 2021. It is now read-only.

exposed view_vector as kwarg for SVG export #232

Merged
merged 6 commits into from
Mar 6, 2018

Conversation

adam-james-v
Copy link
Collaborator

Making a PR to address #213 .
The view vector has been removed already, so this kwarg just exposes the option to re-orient the SVG output

@coveralls
Copy link

coveralls commented Feb 27, 2018

Coverage Status

Coverage remained the same at 91.817% when pulling e6afcca on svg_orientation_control into bb2a7b1 on master.

@dcowden
Copy link
Owner

dcowden commented Feb 27, 2018

This looks ok to me. the change to exporters.py is not reverse compatible but as that's an internal file I think that's ok.

I didn't merge because tests are failing. I've kind of lost track of where we are with that... At one point we knew some tests were failing due to in process python 2/3 stuff, but I'm not sure now

@adam-james-v
Copy link
Collaborator Author

Well, there are errors with GetSVG(), looks like it may be my fault, so I'll have to take a look

Adam (Rusty) Vermeer added 2 commits February 27, 2018 07:28
giving a default value here should allow you to call getSVG() with only one arg. Then, tests should pass
sorry. typo.
@jmwright
Copy link
Collaborator

the change to exporters.py is not reverse compatible but as that's an internal file I think that's ok.

@dcowden can you explain that to me? There are defaults set for the parameters, so existing scripts will run fine. Or do you mean that the SVG can't be imported properly again once exported using this option?

@jmwright
Copy link
Collaborator

@RustyVermeer Thanks for putting this PR together.

@dcowden
Copy link
Owner

dcowden commented Feb 27, 2018

oh @RustyVermeer you're right. when i looked at it earlier, i thought i saw a new param without a default, but i see they have defaults. I was on my phone, maybe it was cut off. sorry about that, looks good.

@adam-james-v
Copy link
Collaborator Author

I'm looking at this on my local machine. I don't want to keep making silly commits. Sorry for this, everyone. Thought this was in better shape than it is

@dcowden
Copy link
Owner

dcowden commented Feb 27, 2018

@RustyVermeer no worries! Let he who has not made silly commits complain ;) I'm thankful you're working on it given what the project pays.

@adam-james-v
Copy link
Collaborator Author

@dcowden @jmwright This PR has no conflicts.
I think it is ok to merge. The only potential concern I saw was the following:

def getSVG(shape, opts=None, view_vector=(-1.75, 1.1, 5.0))

and

def toSvg(self, opts=None, view_vector=(-1.75,1.1,5)):

both have opts. Users might be confused as to why view_vector does NOT exist in opts.

Reasoning I have for keeping them separate is:

  • opts controls the width and height of the svg, which are specific settings for the document
  • view_vector controls the orientation of the model itself, which is a model setting, not a document setting.

At least, that makes sense in my brain :)

@jmwright
Copy link
Collaborator

jmwright commented Mar 5, 2018

@RustyVermeer Thank you for this contribution. Your logic for keeping view_vector separate from opts makes sense to me. @dcowden unless you have any objections I'll go ahead and merge this.

Copy link
Collaborator

@jmwright jmwright 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 to me.

@jmwright jmwright merged commit eb199ba into master Mar 6, 2018
@jmwright
Copy link
Collaborator

jmwright commented Mar 6, 2018

Thanks again @RustyVermeer !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants