Skip to content

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

Merged
timmc-edx merged 3 commits into
masterfrom
timmc/course-asset-view-cleanup
Sep 30, 2024
Merged

refactor: Clean up after conversion of contentserver to view#35559
timmc-edx merged 3 commits into
masterfrom
timmc/course-asset-view-cleanup

Conversation

@timmc-edx

@timmc-edx timmc-edx commented Sep 30, 2024

Copy link
Copy Markdown
Contributor

Last bit of work planned for #34702

Best reviewed commit-by-commit. First commit is an indentation change that results in broken code; second commit fixes things up; third commit addresses some old lint while we're in there.

Remove class declaration and simply de-indent contents by 4 spaces.

This results in broken code due to lingering `self` references, but
should make diffs easier to understand.
Some lingering cleanup from the transition from a middleware to a view.
See #34702 for context.

- Remove IMPL and self/StaticContentServer references
- Add newlines to satisfy code style linter
- Fix test references
- Update module docstring
Several now-irrelevant lint-disables and one legit one that was easy to
fix.
try:
content = AssetManager.find(location, as_stream=True)
except (ItemNotFoundError, NotFoundError): # lint-amnesty, pylint: disable=try-except-raise
raise

Copy link
Copy Markdown
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
Copy Markdown
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.

@timmc-edx
timmc-edx merged commit a100166 into master Sep 30, 2024
@timmc-edx
timmc-edx deleted the timmc/course-asset-view-cleanup branch September 30, 2024 19:54
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants