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.

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

Conversation

timmc-edx
Copy link
Contributor

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

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
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.

@timmc-edx timmc-edx merged commit a100166 into master Sep 30, 2024
49 checks passed
@timmc-edx timmc-edx deleted the timmc/course-asset-view-cleanup branch September 30, 2024 19:54
@edx-pipeline-bot
Copy link
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
Contributor

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

@edx-pipeline-bot
Copy link
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
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
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