kennethreitz/maya

modifying _EPOCH_START can alter the results of MayaDT

bsdtux opened this issue · 3 comments

While doing a code reading session we (Chad Upjohn, Hameed Gifford, and myself) found that a user could import the maya package and change the _EPOCH_START tuple arbitrarily. We then found that this rendered incorrect results with MayaDT

To secure that value from this I am sending in a PR that will make _EPOCH_START a MayaDT class variable.

The thought behind this was since _EPOCH_START is used only by MayaDT then it might make sense to place it here instead of at the module level

a user could import the maya package and change the _EPOCH_START tuple arbitrarily.

Yes, that's right. The _EPOCH_START variable is considered protected / private by convention due to the leading underscore. If a user thinks this variable needs to be changed, the user should know what the impacts are - because it's not public!

The thought behind this was since _EPOCH_START is used only by MayaDT then it might make sense to place it here instead of at the module level

To just secure the variable this doesn't really help. Consider this example:

>>> class A:
...     _B = 1
...     __C = 2
... 
>>> A._B
1
>>> A._A__C
2

However, for me it would be fine to define it as class attribute of MayaDT, but only for the reason that it's solely used there - not because of safety nor security.

Just out of curiosity: why did you want to change this variable in the first place? Did you assume it's some kind of feature? Did you need a workaround for something? I'm just trying to understand if the API is missing something somewhere :)

Great question. I would say we solely wanted to changed it on the basis that it would help protect that variable just a little bit more. Very true in your example that they could still change it which was something that we thought of. However I feel our intent was to provide one more layer of protection around that Global Variable.

You did bring up an interesting byproduct that none of us had considered. Which would be that bringing it into the class would probably have more benefit from a meaningful purpose versus security benefit.

To answer the other two questions I don't feel anything was missing anywhere. Too be honest it was a joy to read through this code for our groups first ever code reading session and one of the reasons we picked it was because of how compact and easily read this code is.

Too be honest it was a joy to read through this code for our groups first ever code reading session

That's nice to hear 🍻

Does anyone else have strong opinion about this?
If there are no objections I'll merge #130