On Wed, Jul 13, 2016 at 9:27 PM, Anders Bertelrud <[email protected]> wrote:
> Thanks for taking the initiative for this, Ankit. It's a very welcome > improvement. > > Comments inline. > > On 2016-07-12, at 11.15, Ankit Agarwal via swift-build-dev < > [email protected]> wrote: > > To mark a target as exported/public I propose PackageDescription's Target > gains > a flags property which would be a Set of the following Flag enum declared > inside Target class: > > public enum Flag { > /// Makes the target public or "exported" for other packages to use. > case public} > > The Flag enum will be flexible in case we need to add more attributes in > future as opposed to a boolean property to mark the public nature of the > target. > > I would prefer that this be a boolean parameter rather than a generic > `flags` parameter, since it makes the manifest read more naturally, and, > importantly, is no less extensible than an enum. Additional parameters > with default values can as easily be added as more enum cases can, and in > either case, a manifest written to assume the existence of `public` will be > equally incompatible with older versions of the package manager. > > So, for example: > > let package = Package( > name: "FooLibrary", > targets: [ > Target(name: "FooLibrary", public: true), > Target(name: "SampleCLI", dependencies: ["FooLibrary"]), > ]) > > We can keep some obvious defaults for targets which can be implicitly > public for e.g. > > 1. Package has only one target. > 2. Target with same name as package. > > I'm a bit wary of magic here. I think it would be clearer to have the > manifest declare what is public and what is not. With magic naming > conventions it's too easy to accidentally change semantics just by renaming > a target. > I agree that we should avoid too much magic, I think you missed the sentence below those examples in the proposal where I mentioned that we should avoid those instead. However as Daniel mentioned we don't want to overcomplicate the manifest for simple packages and for beginners, keeping that in mind we updated the proposed solution which you can find on the proposal link ( https://github.com/aciidb0mb3r/swift-evolution/blob/swiftpm-module-access-control/proposals/xxxx-swiftpm-target-access-control.md ) > I propose that enum Target.Dependency gains a new case External(package: > String, target: String) to declare dependency on an external package's > target. > > Since it's the same fundamental kind of dependency in either case, would > it be better to have `package` be an optional parameter to Target? > > So that `Target(name: "Foo")` is local but `Target(name: "Foo", package: > "Bar")` external? That would seem more logical. > > I like `Target(name: "Foo", package: "Bar")` but I prefer package name is stated before the target name `Target(package: "Foo", name: "Bar")` but then this gets weird and `Target(package:target)` is also weird so I chose `External(package:target)` instead. However if people prefer `Target(name:package)` more then I am fine with it. -- Ankit
_______________________________________________ swift-evolution mailing list [email protected] https://lists.swift.org/mailman/listinfo/swift-evolution
