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

refactor: Clean up after conversion of contentserver to view #35559

Merged
merged 3 commits into from
Sep 30, 2024
Merged

refactor: Clean up after conversion of contentserver to view #35559

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b162b4
refactor: Lift methods out of StaticContentServer (unchanged and broken)
timmc-edx Sep 30, 2024
660c30b
refactor: Fix content server methods and tests (finish method lift)
timmc-edx Sep 30, 2024
7b1519f
style: Fix some lint issues that had been amnestied
timmc-edx Sep 30, 2024
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
1 change: 0 additions & 1 deletion openedx/core/djangoapps/contentserver/test/test_contentserver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ def test_range_request_multiple_ranges(self):
"""
first_byte = self.length_unlocked / 4
last_byte = self.length_unlocked / 2
# lint-amnesty, pylint: disable=bad-option-value, unicode-format-string
resp = self.client.get(self.url_unlocked, HTTP_RANGE='bytes={first}-{last}, -100'.format(
first=first_byte, last=last_byte))

Expand Down
7 changes: 2 additions & 5 deletions openedx/core/djangoapps/contentserver/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def process_request(request):
"""Process the given request"""
asset_path = request.path

if is_asset_request(request): # lint-amnesty, pylint: disable=too-many-nested-blocks
if is_asset_request(request):
# Make sure we can convert this request into a location.
if AssetLocator.CANONICAL_NAMESPACE in asset_path:
asset_path = asset_path.replace('block/', 'block@', 1)
Expand Down Expand Up @@ -294,10 +294,7 @@ def load_asset_from_location(location):
content = get_cached_content(location)
if content is None:
# Not in cache, so just try and load it from the asset manager.
try:
content = AssetManager.find(location, as_stream=True)
except (ItemNotFoundError, NotFoundError): # lint-amnesty, pylint: disable=try-except-raise
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

What was this doing, and did you remove it because it should do the same with and without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's basically a no-op -- the lint documentation notes that a bare raise as the first line of a sole except block is pointless. (Raising something else, or having multiple except blocks with different behavior, or having other stuff before the raise -- those would not be covered by this lint.)

It never did anything other than this, so I think it was imitating some similar code that instead of raising would return a 404.

content = AssetManager.find(location, as_stream=True)

# Now that we fetched it, let's go ahead and try to cache it. We cap this at 1MB
# because it's the default for memcached and also we don't want to do too much
Expand Down
Loading