Reviews are an important part of the Swift evolution process. All reviews should be made in this thread on the Swift forums or, if you would like to keep your feedback private, directly in email to me as the review manager.
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to the Swift Package Manager?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages, libraries, or package managers with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I cannot find mention of whether or not the compiler has a right to optimize resources away. @Aciidsaid it made sense to him in the draft thread, but that he wanted to ask others’ thoughts first. Was it ever decided?
Is it possible to include a .swift or .c file as a resource? One use case would be template files, like the ones SwiftPM produces for you with swift package init. Should it be possible to register a path ending in .swift with copy(_:) such that it would then be both included as a resource and prevented from being compiled into the target binary? (Or is there such thing as a use case for being both a resource and a compiled source at the same time?)
I don't think we made an explicit decision, but since the only supported way to access the bundle is through the generated API, I think this can be left as an implementation detail.
Yes, using copy(_:) will work for that, but it'll override the built-in rule, so the file will not be compiled at this point. This sentence is supposed to cover this case:
However, the package can override the default rule for this file by adding it to the exclude list or by specifying the "copy" rule for this file.
Maybe we should clarify that "default rule" includes compilation here?
Localization support will build on top of the resources feature described in this proposal. We think resources and localization support are separable and it would be better to discuss localization in its own proposal.
As an iOS developer, does this mean NSBundle will not find localized storyboards or xibs until a future proposal?
As an iOS developer, does this mean I can't use NSLocalizedString to return localized strings until a future proposal?
Fabulous, well-considered proposal. The syntax for packages and in package code. I'm happy where the spelling of Bundle.module landed having followed since the pitch was posted.
This composes well with the existing ecosystem without a lot of upheaval, and this will clear the bar for most of the projects I use to use SwiftPM fully.
The only thing I can think that would be missing from this first pass would be for Resource's static constructors to also accept _ condition: BuildSettingCondition? = nil (primarily, in my mind, to get .when(configuration: .debug)), but that can be added later in another proposal if necessary.
One question: What is the intended API for accessing a Package's bundle from outside the package? Will it still be possible to use Bundle.init(identifier:) with the identifier being the package name? Maybe it's up to the Package to expose its Bundle.module with a public accessor?
Because this is an internal static property, it would be visible only to code within the same module, and the implementations for each module would not interfere with each other…Any package target that really wants to just vend the bundle of resources could implement a single property to publicly expose the bundle to clients. It seems reasonable that the package authors have to explicitly vend them.
The identifier is an implementation detail. It will not be the package name. Do not rely on those initializers, even from within the target.
That’s what @jawbroken’s quote is saying. Nothing is supposed to be able to access it from outside the target. If the target wants that, it must do something like public let myPackageBundle = Bundle.module.
I like a Package being able to decide whether to make its Bundle available. I can also see a good argument for adding a standardize way to accomplish this. I can imagine quite a few organization patterns where this would become the norm. E.g. having large binary resources for an application put in a separate target/package or a separate Package with resources that are shared between applications. Standardizing that access would enable tooling and libraries to integrate with a reliable API.
However, adding this would be additive so could be part of a later pitch if such patterns became popular.
Regardless, I can't wait to use the feature and I really like the way the API design turned out!
Yes, it is also true of init(for:). Towards the end of the Motivation section, the proposal says this:
For example, the source code cannot assume that the resources will be in the same bundle as the compiled code (if bundles even exist as separate entities on a given platform).
That's a potential reason to not use the API, it doesn't mean it won't work the same way it currently does when e.g. using a framework's bundle in an iOS application. Also a Class is not an obfuscated implementation detail like a bundle Identifier would be.
It means that once the package is built into something, the resources may not be in the same bundle as the class. (In fact they almost certainly will not. Libraries are statically linked by default, so the class will most often end up in the main bundle. But if resources defaulted to being splatted into the main bundle, there would be a massive risk of name clashes.)
If you query the Bundle you got from init(for:) asking after a named resource, you could end up with something different than you expect, or nothing at all. Do not try to use it that way.
It is not the class that is the implementation detail. The implementation detail is which bundles the various pieces of the product wind up in—most importantly with respect to the compiled binary vs the bundled resources.
All you can trust is that Bundle.module will point at wherever the package manager decided to put your resources during that particular build.
I did a quick read and like the proposal. It will work for my use of resources excluding the localization. I do agree that it will be better to have a separate discussion about localization.
I like the approach with multi-purpose file types. I have always kept internal documents away from target directories just to be safe when shipping, but the proposal makes it possible to handle the documents in a different way too.
The API is nice. The proposal takes the SwiftPM to a right direction.
I have the same question. Can someone elaborate on why resource and localization support are "separable"? If this will accepted and we'll able to do let localized = NSLocalizedString("key", bundle: Bundle.module), then what does it mean?
it would be better to discuss localization in its own proposal.
Probably beyond the scope of this proposal, but I’ll throw this out there. Do resources have to be paths to files assumed to be local than rather than URLs potentially remote?
If Resources was a protocol and the process and copy functions took a continuation which themselves took a concrete Resource value, we could quite easily get a little more creative with resources, like switching, generating or lazily loading them from a separate location etc.