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

Make it possible to use multiple calendar settings in the same app. #6

Open
pmav99 opened this issue Feb 8, 2018 · 5 comments
Open
Labels

Comments

@pmav99
Copy link
Contributor

pmav99 commented Feb 8, 2018

My current usecase does not fall in this category, but a relatively subtle implication of the current implementation is that it is not really possible to use different fiscal calendar settings in the same application. The problem is that as soon as you update the global settings, the behaviour of the existing objects changes too. For example:

In [2]: from fiscalyear import *

In [3]: y = FiscalYear(2018)

In [4]: y.start
Out[4]: FiscalDateTime(2017, 10, 1, 0, 0)

In [5]: setup_fiscal_calendar('same', 1, 1)

In [6]: y.start
Out[6]: FiscalDateTime(2018, 1, 1, 0, 0)

Arguably, someone could claim that this is a feature too! :P
And in certain contexts, it probably is! Neverheless, it can also lead to subtle bugs and various errors when you do calculations. If nothing else, I think that there should be at least a warning in the docs.

PS. The older I get the more I like immutable objects :P

@adamjstewart
Copy link
Owner

It is actually currently possible to do this using the fiscal_calendar context manager. See https://fiscalyear.readthedocs.io/en/latest/advanced_usage.html#temporarily-changing-the-fiscal-calendar

@pmav99
Copy link
Contributor Author

pmav99 commented Feb 8, 2018

I am probably at fault, since I mixed two different issues here.

  1. I agree that you can use the context manager, even though i am not sure an API like this can scale if you have multiple settings. It feels rather error-prone.

  2. Regardless of the previous point, changing how existing objects behave is IMHO not optimal. IMHO changing the setting should only affect new objects. Again, I don't have a strong opinion for this, since in my current use case I will only need to set the settings once.

@adamjstewart
Copy link
Owner

That's true, we could redesign the API so that things like START_YEAR, START_MONTH, and START_DAY are attributes of every single object we create. Honestly, when I was designing this, I assumed that the majority of people would be working with a single fiscal calendar, and that it would be pretty infrequent that you would have to mix and match fiscal calendars. In reality, this might be more common than I expected.

Do you know of any other Python libraries that allow you to change global variables? It would be interesting to compare my implementation to how others do it. I'm not opposed to changing it to what you propose, but I don't want to change it without a good reason, and I want to make sure I'm changing it in a way that will be stable and maintainable.

@pmav99
Copy link
Contributor Author

pmav99 commented Jun 14, 2019

I would suggest to completely avoid globals if possible. They do make it harder to test things etc.

@galenseilis
Copy link

I could see some statistical forecasting products breaking with globals such as START_YEAR, START_MONTH, and START_DAY. Easy to avoid if you know about these globals, but could trip the occasional person.

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

No branches or pull requests

3 participants