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

Remove 'x' and 'y' aliases from Dimension enumeration #5225

Merged
merged 2 commits into from Sep 29, 2016

Conversation

@mattpap
Copy link
Contributor

@mattpap mattpap commented Sep 23, 2016

fixes #5223

@canavandl
Copy link
Contributor

@canavandl canavandl commented Sep 25, 2016

do we need to add a deprecation or did this the x and y dimension never work anywhere?

@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 28, 2016

Actually we should go further. Dimension enumeration is used (most of the time) in List(Enum(Dimension)) properties. This is cumbersome, because the only possibilities are either width, or height or both (so nonexistent width_height, which is the default). I presume empty list, so no dimensions, is technically possible, but doesn't make sense (e.g. pan in no dimension means it is disabled, but that's a pretty bad way of expressing this fact). @bryevdv?

@mattpap mattpap force-pushed the mattpap/5223_dimension_x_y branch from 4d58f54 to 282ef31 Sep 28, 2016
@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 28, 2016

@bryevdv, I went ahead and made appropriate changes. Added proper deprecation as well. After this change, a lot of code looks more intuitive.

@mattpap mattpap force-pushed the mattpap/5223_dimension_x_y branch from 282ef31 to 24b807f Sep 28, 2016
@bryevdv
Copy link
Member

@bryevdv bryevdv commented Sep 28, 2016

Agreed this is better.

@canavandl canavandl mentioned this pull request Sep 28, 2016
2 of 3 tasks complete
@canavandl
Copy link
Contributor

@canavandl canavandl commented Sep 28, 2016

LGTM - i spot tested the affected tools and annotations.

@mattpap mattpap force-pushed the mattpap/5223_dimension_x_y branch from 24b807f to f23acc2 Sep 29, 2016
@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 29, 2016

@canavandl, thanks.

@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 29, 2016

I had to repush, because I forgot to update typings.

@mattpap mattpap merged commit d2c7191 into master Sep 29, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mattpap mattpap deleted the mattpap/5223_dimension_x_y branch Sep 29, 2016
mattpap added a commit that referenced this pull request Sep 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.