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

Fix file extension detection for files with multiple dot separators #1408

Merged
merged 3 commits into from
Jan 18, 2025

Conversation

nerlihmax
Copy link
Contributor

Hi!

I have issues with the flag --add-encoding-exts=js,html,css when uploads js static on production buckets. My files have names like widget-no-csr.oWiXNN3Poy.js or widget-no-csr.oWiXNN3Poy.js.gz, where is more than one dot symbol separator. Because

s3cmd/S3/S3.py

Line 736 in 10aea09

parts = filename.rsplit('.',2)
there is file extension oWiXNN3Poy for first file and js for second. That looks like a bug. I fixed that, but I think it could be kind of dangerous because it breaks backward compability.

@fviard
Copy link
Contributor

fviard commented Jan 13, 2025

Thank you for your PR, indeed the existing code looks very strange, and my best guess is that it was not done on purpose but a confusion with the count argument of the python split.

S3/S3.py Outdated Show resolved Hide resolved
@fviard
Copy link
Contributor

fviard commented Jan 13, 2025

And btw, don't worry about the automatic test failing, it is not related to your PR :-)

@snosratiershad
Copy link
Contributor

@nerlihmax, @fviard LGTM.

@fviard fviard merged commit 9f4cbe6 into s3tools:master Jan 18, 2025
6 of 7 checks passed
@fviard
Copy link
Contributor

fviard commented Jan 18, 2025

Merged, thank you.
@nerlihmax For info if you do other PR, it would be good to "clean" or "squash" the commits in your PR for them to be clean once merged.
Here I did with a "squash" through "github" but it is always better if the PR can be merged as-is :-)

@fviard fviard added this to the 2.4.1 milestone Jan 18, 2025
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