Skip to content

implement package-collections business logic #3028

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

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Nov 6, 2020

motivation: continue the implmentation of package-collections

changes:

  • implemented PackageCollectionsProtocol which is the main business logic of the feature
  • added basic configuration model for the feature
  • added basic validation rules for collection source
  • simplfied model names to be shorter
  • added relevants tests

@tomerd tomerd requested review from yim-lee and removed request for aciidgh November 6, 2020 08:27
Copy link
Contributor

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

Looks great. Minor comments.

neonichu added a commit to neonichu/swift-package-manager that referenced this pull request Nov 6, 2020
This implements the proposed CLI for package-collections.

Requires swiftlang#3028
@tomerd
Copy link
Contributor Author

tomerd commented Nov 7, 2020

@swift-ci please smoke test

@tomerd tomerd force-pushed the feature/package-collections-2 branch from 347f658 to 0076c14 Compare November 7, 2020 03:10
@tomerd
Copy link
Contributor Author

tomerd commented Nov 7, 2020

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Nov 7, 2020

hmm test failures are because of API changes in TSC: swiftlang/swift-tools-support-core#152

@tomerd
Copy link
Contributor Author

tomerd commented Nov 7, 2020

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Nov 7, 2020

@yim-lee @abertelrud @neonichu this should be ready for final review / merge


extension PackageCollectionsModel.CollectionSourceType: Codable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the whole purpose of this just to be able to throw UnknownType error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yim-lee are you asking about the purpose of CollectionSourceType? If so, it for us to be able to support multiple type of providers. e.g. feed, index, etc

Copy link
Contributor

@yim-lee yim-lee Nov 9, 2020

Choose a reason for hiding this comment

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

Can we use synthesized conformance and remove this extension block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I dont believe enums synthesize conformance. am I missing anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could if CollectionSourceType: String, Codable but not sure we want to add : String though.

@tomerd
Copy link
Contributor Author

tomerd commented Nov 10, 2020

@swift-ci please smoke test

motivation: continue the implmentation of package-collections

changes:
* implemented PackageCollectionsProtocol which is the main business logic of the feature
* added basic configuration model for the feature
* added basic validation rules for collection source
* simplfied model names to be shorter
* added relevants tests

Co-authored-by: Boris Bügling <[email protected]>
Co-authored-by: Yim Lee <[email protected]>
@tomerd tomerd force-pushed the feature/package-collections-2 branch from 808d351 to c7fc365 Compare November 10, 2020 00:18
@tomerd
Copy link
Contributor Author

tomerd commented Nov 10, 2020

@swift-ci please smoke test

@tomerd tomerd self-assigned this Nov 10, 2020
@tomerd tomerd merged commit 93f850f into swiftlang:main Nov 10, 2020
@tomerd tomerd deleted the feature/package-collections-2 branch November 10, 2020 01:54
neonichu added a commit to neonichu/swift-package-manager that referenced this pull request Nov 12, 2020
This implements the proposed CLI for package-collections.

Requires swiftlang#3028
neonichu added a commit to neonichu/swift-package-manager that referenced this pull request Nov 20, 2020
This implements the proposed CLI for package-collections.

Requires swiftlang#3028
neonichu added a commit to neonichu/swift-package-manager that referenced this pull request Dec 11, 2020
This implements the proposed CLI for package-collections.

Requires swiftlang#3028
neonichu added a commit that referenced this pull request Dec 11, 2020
* Add a new `swift-package-collections` CLI

This implements the proposed CLI for package-collections.

Requires #3028

* address feedback

* address feedback and adopt to latest API

* more feedback

* Update with proposal changes

* Fix output indentation and rename CLI tool

* Fix JSON output and adopt `.makeWithDefaults()`
federicobucchi pushed a commit to federicobucchi/swift-package-manager that referenced this pull request Jan 6, 2021
motivation: continue the implmentation of package-collections

changes:
* implemented PackageCollectionsProtocol which is the main business logic of the feature
* added basic configuration model for the feature
* added basic validation rules for collection source
* simplfied model names to be shorter
* added relevants tests

Co-authored-by: Boris Bügling <[email protected]>
Co-authored-by: Yim Lee <[email protected]>
federicobucchi pushed a commit to federicobucchi/swift-package-manager that referenced this pull request Jan 6, 2021
* Add a new `swift-package-collections` CLI

This implements the proposed CLI for package-collections.

Requires swiftlang#3028

* address feedback

* address feedback and adopt to latest API

* more feedback

* Update with proposal changes

* Fix output indentation and rename CLI tool

* Fix JSON output and adopt `.makeWithDefaults()`
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.

4 participants