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: added support for custom file extension with multiple parts #710

Merged
merged 3 commits into from
Feb 20, 2023

Conversation

tolgaeren
Copy link
Contributor

Description

This PR modifies the single_file.py to add support for multiple extension parts. An example is setting "tar.gz" as the custom _file_extention string:

class DotInFileExtension(SingleFileSnapshotExtension):
    _file_extension = "tar.gz"

Previously this was causing some unexpected behavior as described in #703 . I've found the problematic part in SingleFileSnapshotExtension._read_snapshot_collection in single_file.py:

 snapshot_collection.add(Snapshot(name=Path(snapshot_location).stem))

.stem only removes the last extension not all the extensions as documented. We would like to remove as many extensions as there are in the _file_extension string.

There are a few ways to fix it:

  1. Calling .stem as many times as number of extensions.
  2. Calling .with_suffix("") as many times as the number of extensions.
  3. Replacing the self._file_extension at the end of snapsot_location with "".

I tried option 3 first, it seemed to break a lot of other tests. Then I picked option 1 as it seemed it was the closest to original implementation. Let me know if you think of a better way to do it.

Added a test under test_single_file.py as it seemed like the right location for it.

Related Issues

Checklist

  • This PR has sufficient documentation.
  • This PR has sufficient test coverage.
  • This PR title satisfies semantic convention.

Additional Comments

No additional comments.

@tolgaeren tolgaeren changed the title fix: Added support for custom file extension with multiple parts fix: added support for custom file extension with multiple parts Feb 16, 2023
@noahnu
Copy link
Collaborator

noahnu commented Feb 19, 2023

Ignore the benchmark failure, it's a permissions issue. I'll try take a look at this PR in the next couple days. Rather than loop through stems, I feel like there should be some sort of "basename without user supplied extension" function in the python stdlib.

@tolgaeren
Copy link
Contributor Author

"basename without user supplied extension"

I don't like the loop either. I looked for this but couldn't find it😀 if I can find some time tomorrow, I'll check the stem implementation and get some inspiration from that 😅

from pathlib import Path


def test_multiple_file_extensions(testdir):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't make your test fail on main, so I've replaced it with more of an integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you couldn't trigger the test because of a typo:

class DotInFileExtension(SingleFileSnapshotExtension):
    _file_extension = "{file_extension}"

The f was missing before the f-string :) That was fixed when you moved it into an integration test setup.

Anyways, thank you for the merge!

@noahnu noahnu merged commit efe687e into syrupy-project:main Feb 20, 2023
@noahnu
Copy link
Collaborator

noahnu commented Feb 20, 2023

@all-contributors add @tolgaeren for bug

@allcontributors
Copy link
Contributor

@noahnu

I've put up a pull request to add @tolgaeren! 🎉

tophat-opensource-bot pushed a commit that referenced this pull request Feb 21, 2023
## [4.0.1](v4.0.0...v4.0.1) (2023-02-21)

### Bug Fixes

* **serializer:** handling of multi-part file extensions in SingleFileExtension ([#710](#710)) ([efe687e](efe687e))
@tophat-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 4.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

having a dot in the file extension causes unexpected behavior
3 participants