You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
At Python Study Group this week, we looked at context managers. When we got to
@contextlib.contextmanager
, we learned that this is wrong: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):
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
The text was updated successfully, but these errors were encountered: