Find incorrect context managers
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 52
- Forks
- 33
- PR merge metrics
- No merged PRs in 30d
Description
At Python Study Group this week, we looked at context managers. When we got to @contextlib.contextmanager, we learned that this is wrong:
@contextmanager
def my_context_manager():
#... set up
yield
#... clean up
because the clean up won't execute if an exception happens inside the caller's with-statement.
I grepped edx-platform to see if this happens, and it does! here and here for example.
We also have instances of pointless context managers with no clean up at all (here):
@contextmanager
def lti_consumer_fields_editing_flag(course_id, enabled_for_course=False):
"""
Yields CourseEditLTIFieldsEnabledFlag record for unit tests
Arguments:
course_id (CourseLocator): course locator to control this feature for.
enabled_for_course (bool): whether feature is enabled for 'course_id'
"""
RequestCache.clear_all_namespaces()
CourseEditLTIFieldsEnabledFlag.objects.create(course_id=course_id, enabled=enabled_for_course)
yield
It would be cool to lint for these mistakes.
BTW: A reason to have a cleanup-less context manager is in a method overriding a correct context manager, but the subclass needs no clean up. If we write this linter, that case can use a disabling pragma.
/cc @cpennington @jmbowman @BessieSteinberg
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the context-manager examples in common/test/acceptance/pages/lms/discussion.py, common/test/utils.py, and cms/djangoapps/xblock_config/tests/test_models.py. Define how the linter should identify exception-unsafe cleanup and cleanup-less managers, including a disabling pragma for valid overrides; done means these cases are detected without rejecting that exception.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100