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.

Sign up for GitHub

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

Jump to bottom

docs: update oep-49 for clarity around new apps #455

Closed
wants to merge 3 commits into from
Closed

docs: update oep-49 for clarity around new apps #455

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b1f15b
docs: move oep-49 to best practices and add history
Mar 1, 2023
15d51a7
docs: add recommendation for ADR
Mar 1, 2023
82b3eae
Merge pull request #455 from openedx/rsgraber/20230301-move-oep-49
Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion ...ecisions/oep-0049-django-app-patterns.rst → ...ractices/oep-0049-django-app-patterns.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ OEP-49: Django App Patterns
* - Status
- Accepted
* - Type
- Architecture
- Best Practice
* - Created
- 2021-01-29
* - Review Period
Expand Down Expand Up @@ -267,7 +267,36 @@ Do note that even if you expose your tasks through ``api.py`` to be used by othe
still be configured with the ``tasks`` import name, as the celery identifier for your task (as set by the celery
decorator) is based off the original file.

docs/decisions/0001-purpose-of-this-app.rst
===========================================

This should be an architectural decision record (ADR) describing the decision behind adding the app. Future ADRs should also be placed in this directory. See more about what should go in ADRs in the `ADRs section of OEP-0019`_.

If this is the only app in the repository, this ADR should just be a stub linking to the full ADR in `0001-purpose-of-this-repo.rst` (see `TODOs after running cookiecutter`_).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding this to the post-cookiecutter TODO list, could we add it to the cookiecutters as a part of this OEP change?

Copy link
Contributor Author

@rgraber rgraber Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in postpone this PR until we have updated the cookiecutter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, or at least until we have a draft PR ready to go for updating the cookiecutter. Since we are asking folks to make these additional ADRs starting now, my intuition is that it'd be best to provide the cookiecutter scaffolding now as well.

I don't know, maybe I'm underestimating the effort it'd take to update the cookiecutter. Don't take this as a blocking suggestion; what you have here is OK with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm nervous about blocking this, so I'll leave this as is and then make an issue to add this to the cookiecutter if there isn't one already. If there is one I'll add a task to update this line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the benefit of blocking on this. The OEP states what someone should do. Cookiecutter changes may help automate some of that, but I bet there are a ton of automation improvements in our OEPs, and this is just one.

Also, one could argue that adding the stub automatically will make it less likely that anyone will update it in the many cases where it should be updated. An alternative would be to add a codeblock here with an example link, if someone works out the rST reference that would work. Note, this too could come later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now openedx/edx-cookiecutters#299. We can further discuss there.


.. _ADRs section of OEP-0019: https://open-edx-proposals.readthedocs.io/en/latest/oep-0019-bp-developer-documentation.html#adrs
.. _TODOs after running cookiecutter: https://github.com/openedx/edx-cookiecutters#3-todos-after-running-cookiecutter

Consequences
************

At this time, there is no plan to enforce any of these guidelines. The vast majority of current Open edX code doesn't yet meet these guidelines, and there will always be exceptions to the rule. The hope is that as developers write new code or refactor existing code, they follow these patterns as best they can. We also hope that code reviewers will ensure these guidelines are followed in the code they approve.

Change History
**************

2023-03-01
==========

* Changed type from "Architecture" to "Best Practice"
* Added section for an ADR justifying the new app

2021-04-23
==========

* Accepted

2021-04-12
==========

* Initial publication