Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add timezone setting along with menu of choices at project creation #139
Add timezone setting along with menu of choices at project creation #139
Changes from 2 commits
965186e40fe0ed96273a9a34d19d26779df59131f0File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is set explicitly upstream: https://github.com/girder/django-composed-configuration/blob/e65bdc513d74ead19035a837079dad6df5ddf48c/composed_configuration/_django.py#L83
At a minimum, if we agree that
"America/New_York"is a better default than"UTC", we can just change that upstream.Furthermore, I agree that timezone may be a common thing which individual projects want to override. As a less disruptive change, which would not require a new cookiecutter option, we could just turn this line into a code comment, which would inform downstreams that they could trivially override the
TIME_ZONEfor their local project. The benefit is that it would defer this choice (which is not absolutely essential to starting up a working project) to a later point in the lifecycle, while still providing a hopefully-obvious pointer to one of the first settings which developers may actually want to override.While we're on the subject, we should perhaps think of other settings that it's highly likely developers want to override, and put pointers to them in here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a short term action item, should I just close this PR and open a one-line-change PR in
django-composed-configuration?As a longer term thing, how do I learn about the relationship between this cookiecutter repo and django-composed-configuration? Is there documentation, or can I just bug you on Slack about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repo produces a working Django / Girder 4 project. To do that, this repo makes some opinionated choices about the dependencies it uses (including Celery, Django Allauth, etc.)
One of the dependencies is
django-composed-configuration, which provides an extensible set of sensible default Django-settings.Once a project is started, downstreams are free to abandon any or all of default dependencies and it's perfectly possible to "eject"
django-composed-configurationby replacing it with a local static list of Django-settings. The advantage to using / keepingdjango-composed-configurationis that as "best practices" around many Django-settings evolves, downstream projects can get the updates for free, without having to manage hundreds of settings (most of which they aren't opinionated about) explicitly.As a meta-note about documentation, I think it makes sense to update
django-composed-configurationdocs to better explain the rationale and value it provides. However, it is ultimately just one library amongst many, and unfortunately, it's probably necessary for developers to have a superficial understanding of almost all the Girder 4 / cookiecutter dependencies and their value proposition in order to successfully further build out a full project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that even if this PR just added a code comment showing how
TIME_ZONEcould be overridden, it would be a nice net improvement to the cookiecutter.The bonus would be if someone could figure out a short list of the most immediate settings which projects would want to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted the offered setting into an example of how to override, and I'll open a PR to change the default timezone in the other project.
I don't have very much insight into what other settings ought to go here, but I'd like to conclude this work and get it off my plate, so maybe instead we can just open an issue asking for discussion on other settings that should be demo'ed in this fashion.