Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
macOS support for swiftly #121
macOS support for swiftly #121
Changes from 10 commits
5d11297
84f969d
43fb8b8
5d86512
472d6ed
c307f5e
61b2e8c
e2d25fb
76d1a08
aab5f99
5ec3bbf
5e073d0
5facaef
8fd3d39
4658330
ece63e7
bad95c2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're bumping to 5.10 we should probably switch on strict concurrency checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm definitely interested in enabling better checks. How does one turn on the strict concurrency checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your target definition in Package.swift add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I tried enabling this and there were a non-trivial number of warnings (errors in Swift 6). I've raised an issue #124 to address the concurrency problems and enable the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth us migration to NIOFileSystem, since we already have NIO as a dependency. Aside from the nicer API, it would be one less thing to use when depending on new Foundation which should make for a smaller binary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can raise this as a separate issue. I believe that there are Foundation dependencies all over the code. Pulling all of that out would be a significant sweep.
I'm not familiar with NIOFileSystem. Something that I really like about FileManager API is that it appears to be very mockable so that we could refactor the tests to operate on a full mock of the filesystem. Is that capability available with NIOFS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear I'm not suggesting we pull out all of Foundation, but minimising it to just
FoundationEssentials
etc could help reduce the size (especially if we don't need ICU).Which specific parts would you use for mocking (not getting into a discussion about that word 🤣 )? NIOFileSystem has a similar API in that you have
FileSystem.shared
you perform the operations on, but the API is much more modern and built with Concurrency in mind (may also help solve some of the issues in #124 )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I agree that NIOFileSystem is more appropriate here since it uses async code to do the work. However, I would defer that from this PR since it is something we can do separately. WDYT @0xTim ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes let's defer this to another PR. I've added an issue #125
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah absolutely shouldn't be a blocker for this PR