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: Batch storage split_url to work with Windows paths #1043

Conversation

BuzzCutNorman
Copy link
Contributor

@BuzzCutNorman BuzzCutNorman commented Oct 5, 2022

A proposed solutions to issue #1042. This fix for StorageTarget.split() adds detection of Windows and the presence of \ in url. If it detects both Windows and \ it splits the url and returns the results. The split logic is a rework of the code from fs.path.split .

Closes #1042


📚 Documentation preview 📚: https://meltano-sdk--1043.org.readthedocs.build/en/1043/

@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.13%. Comparing base (4948150) to head (ee07cb6).
Report is 1094 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
+ Coverage   83.11%   83.13%   +0.01%     
==========================================
  Files          39       39              
  Lines        3743     3747       +4     
  Branches      627      628       +1     
==========================================
+ Hits         3111     3115       +4     
  Misses        470      470              
  Partials      162      162              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

@BuzzCutNorman I've added split_url tests and a marker for Windows. Feel free to change it or add more test cases.

@BuzzCutNorman
Copy link
Contributor Author

BuzzCutNorman commented Oct 6, 2022

@edgarrmondragon I added a few more tests. I am starting to think the ones without the file:// aren't useful since the tap workflow seems to always add that to the url path the target reads. Let me know if you would like me to remove them.

The one that got me today is if I put a valid PyFilesystem url file://c://development/batches in the tap config.json I get a file url of "file://c:\\development\\batches\\test-batch-tap-stackoverflow-sampledata--tags-b1f75d73-e2dc-4033-a62f-99b517ca015d-58.json.gz" in the manifest.

@edgarrmondragon
Copy link
Collaborator

edgarrmondragon commented Oct 6, 2022

I am starting to think the ones without the file:// aren't useful since the tap workflow seems to always add that to the url path the target reads. Let me know if you would like me to remove them.

@BuzzCutNorman yeah let's get rid those for now, you're right that users shouldn't end up with that type of URLs in a normal workflow 👍

@BuzzCutNorman
Copy link
Contributor Author

@edgarrmondragon, The windows-local tests with a url not containing the protocol file:// have been removed.

@edgarrmondragon
Copy link
Collaborator

Thanks @BuzzCutNorman!

@edgarrmondragon edgarrmondragon changed the title fix: StorageTarget split() to work with Windows paths fix: Batch storage split_url to work with Windows paths Oct 6, 2022
@edgarrmondragon edgarrmondragon merged commit 796ccda into meltano:main Oct 6, 2022
@BuzzCutNorman BuzzCutNorman deleted the 1042-StorageTarget-Split-fix-for-Windows branch October 20, 2022 15:41
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.

[Bug]: StorageTarget.split_url() does not return head portion of tuple for Windows file paths
2 participants