Skip to content

Conversation

@TyberiusPrime
Copy link
Contributor

With regard to our discussion in #254,
this is how I would solve the 'fixed-trans-scales-must-raise-on-trans-paramter != their-trans' goal.

All it needs is a mixin base class - the scales were using multiple inheritance already (scale_x_datetime),
so no additional magic needs to be understood (unlike e.g. a solution with a class decorator) and the boilerplate is minimal.

Not sure if you want the base class with @document.

@has2k1
Copy link
Owner

has2k1 commented Feb 1, 2019

I was reluctant to introduce a mixin class because, I wanted to show a warning and a mixin would be too much just for that function. I think only the documentation needs to change.

@TyberiusPrime
Copy link
Contributor Author

I believe warnings are for 'this might be wrong or not, the lib can't tell'-situations, while
obviously wrong (as in "you have told me contradictory things to do") should raise an Exception.

I can see two other straight forward implementations, either with a class decorator (magic), or a
flag on the transformed-scale classes (e.g. use '_fixed_trans' instead of _trans) which scale_continuous then interprets as 'warn/raise on trans'.

(Philosophically, having two ways to define transformations, scale_continous(trans=) and scale_trans is unpythonic - but nothing we can do about that).

@has2k1
Copy link
Owner

has2k1 commented Feb 1, 2019

use '_fixed_trans' instead of _trans

That would work, but I think there is a better way.

@TyberiusPrime
Copy link
Contributor Author

A separate flag?

You might get away with having the test in scale_continuous if _trans != Identity (or waiver...) But it is not clear and poses a restriction for all child classes not just those that opt in.

has2k1 added a commit that referenced this pull request Mar 10, 2019
@has2k1
Copy link
Owner

has2k1 commented Mar 10, 2019

Thanks for raising and exploring this issue, I have resolved it in 0c9f85b with a warning when the trans for non-identity scale is set to a different transformation.

@has2k1 has2k1 closed this Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants