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 incorrect extension being returned #722

Closed
wants to merge 4 commits into from

Conversation

cercos
Copy link
Contributor

@cercos cercos commented Oct 18, 2022

When a filename has multiple dots it returns everything after first dot. os.path.splitext() correctly handles dot names and even '.hidden_linux_file'

When a filename has multiple dots it returns everything after first dot.  os.path.splitext() correctly handles dot names and even '.hidden_linux_file'
@girardinsamuel
Copy link
Contributor

Thanks for this PR. Can you add a unit test to demonstrate that it solves the issue ?

I will review this asap !

@cercos
Copy link
Contributor Author

cercos commented Oct 22, 2022

I will try. I have never created one but I'm interested in the process. Are unit tests universal or do frameworks integrate them in a specific way and is there a specific package I should be using for masonite?

@girardinsamuel
Copy link
Contributor

We are using pytest to run Masonite test suite (tests are based on unittest). There are some handy features available to write Masonite unit tests: https://docs.masoniteproject.com/testing/getting-started.

Here it is easy because tests already exists for this method, so you can edit it: tests/core/utils/test_filesystem.py.

It demonstrates that it works with multiple dots in the extension...so what extension did you tried that did not work here ? 🤔

@cercos
Copy link
Contributor Author

cercos commented Oct 22, 2022

Ok thank you I will read up, didnt even think of looking at the docs lol. The exact filename I was using to test the upload was "archlinux-2022.09.03-x86_64.iso" and it returns ".09.03-x86_64.iso" as the extension.

@girardinsamuel
Copy link
Contributor

I might have found a better solution (more automated) avoiding users to fill an env variable.
I am still using pathlib as it's doing a good job at finding all part of the name which contains . and if the last one is not a known extension I am joining all parts to build an extension.

I have added more tests to show that it solves your problem. If you have tricky extensions in mind feel free to add them to PR comments and I will add them to unit tests to check if this implementation can handle them too...

You can check it out here: #727

@josephmancuso
Copy link
Member

Does #727 succeed this PR?

@girardinsamuel
Copy link
Contributor

Does #727 succeed this PR?

Yes problem solved 💪 !

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