Skip to content

Conversation

@Skyline-23
Copy link

@Skyline-23 Skyline-23 commented Nov 16, 2025

This pull request refactors the Downloader class and related code to remove its dependency on the Combine framework, simplifying its usage and integration. It also clarifies the download progress reporting mechanism and updates documentation accordingly.

Summary of Changes

Dependency Removal and API Updates

  • Removed ObservableObject conformance from the Downloader class in Downloader.swift.
  • Removed the Combine import from Downloader.swift and DownloaderTests.swift.
  • Updated the documentation for the download method in HubApi.swift to indicate that progress is now provided via a callback closure instead of Combine publishers.
  • Improved documentation around error handling and clarified API purpose.

Documentation Improvements

  • Updated comments in HubApi.swift to note that progress reporting occurs through a simple callback.
  • Added guidance that projects needing SwiftUI-style progress updates can wrap the callback with Combine externally if necessary.
  • Added more explicit descriptions for:
    • Progress reporting (percentage and optional download speed)
    • Behavior when cached files are already present

Test Updates

  • Removed all unused Combine imports from DownloaderTests.swift, reflecting the updated implementation.

By removing Combine as a dependency, the downloader becomes more lightweight, easier to integrate, and clearer in how it reports download progress.

Remove unused combine dependency
@Skyline-23 Skyline-23 changed the title Remove Unused Combine Remove unused Combine Nov 16, 2025
@Skyline-23 Skyline-23 changed the title Remove unused Combine Increase Cross-Platform Compatibility for Tokenizer Support on Linux and Windows Nov 16, 2025
@Skyline-23 Skyline-23 closed this Nov 16, 2025
@Skyline-23 Skyline-23 deleted the feature/remove-combine branch November 16, 2025 13:27
Copy link
Collaborator

@mattt mattt left a comment

Choose a reason for hiding this comment

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

Hi @Skyline-23, thanks for opening this PR. I think removing the dependency on Combine for the internal Downloader type is a good change, but I'm not sold on the Crypto change. I don't believe Linux or Windows support is necessarily a goal for this project, so the additional dependency is a hard sell.

If you feel strongly about that, maybe we could decouple these? Merge the quick win for removing Combine, and then open a follow-up that explores swift-crypto as a conditional dependency for Linux and Windows?

@Skyline-23 Skyline-23 restored the feature/remove-combine branch November 16, 2025 13:39
@Skyline-23 Skyline-23 reopened this Nov 16, 2025
@Skyline-23 Skyline-23 changed the title Increase Cross-Platform Compatibility for Tokenizer Support on Linux and Windows Remove unused combine Nov 16, 2025
@Skyline-23
Copy link
Author

I’ve updated the PR title and description to match the purpose of this change.

@mattt mattt changed the title Remove unused combine Remove unused Combine import Nov 16, 2025
Copy link
Collaborator

@mattt mattt left a comment

Choose a reason for hiding this comment

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

LGTM. Thoughts, @pcuenca?

@mattt mattt requested a review from pcuenca November 16, 2025 14:10
Co-authored-by: Mattt <mattt@me.com>
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.

2 participants