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: remove unused import and rename S3Boto3Storage constructor param #39

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

ziafazal
Copy link
Collaborator

This PR has couple of changes to

  1. Remove unused import
  2. Rename bucket param in S3Boto3Storage constructor to bucket_name since it is removed since 1.10. This change does not break anything in older(1.9) version of django-storages.

@ziafazal
Copy link
Collaborator Author

@regisb could you please review this one?

@@ -18,7 +17,7 @@ def __init__(self, xblock, bucket, querystring_auth, querystring_expire):
# No need to serve assets from a custom domain.
self.custom_domain = None
super().__init__(
bucket=bucket,
bucket_name=bucket,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please also modify the S3ScormStorage constructor signature such that it matches the super constructor?

def __init__(self, xblock, bucket_name=None, querystring_auth=None, querystring_expire=None):

@ziafazal ziafazal force-pushed the ziafazal/change-bucket-name-param branch from 3a89ef9 to 20521a1 Compare June 21, 2023 11:58
openedxscorm/storage.py Outdated Show resolved Hide resolved
…param

refactor: match contructors

refactor: reviewer feedback changes
@ziafazal ziafazal force-pushed the ziafazal/change-bucket-name-param branch from 20521a1 to 18abe7a Compare June 21, 2023 12:58
@regisb regisb merged commit a162a60 into overhangio:master Jun 22, 2023
@regisb
Copy link
Contributor

regisb commented Jun 22, 2023

Thanks for the PR!

@awais786
Copy link

@regisb can you please release the new version ?
we are facing issue on production since we have upgraded the django-storages to 1.10.1.

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