Skip to content

Conversation

supersonicbyte
Copy link
Collaborator

@supersonicbyte supersonicbyte commented Aug 31, 2024

Fixes #3297

This PR addresse this issue: #3297

Wrong code snippet shown for non-library products. Additionally, executable products are filtered out and will be addressed in a different PR.

Copy link

cla-bot bot commented Aug 31, 2024

Thanks so much for submitting a pull request. We appreciate your contribution!

We require all contributors to sign our Contributor License Agreement (CLA) before we can merge any code, and we don't currently have a record of a signed CLA for @supersonicbyte. Please email [email protected] and let us know you need to sign a CLA, and we'll get the template over to you.

Copy link
Member

@finestructure finestructure left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @supersonicbyte!

I've left a comment regarding a small thing about using rawValue.

Another thing I noticed when running your changes is that I'm getting a blank snippet:

Screenshot 2024-09-01 at 16 46 55 Screenshot 2024-09-01 at 16 47 20

Does this work for you or is this a problem on my end?

.option(
.data(named: "package", value: package),
.data(named: "product", value: product.name),
.data(named: "type", value: product.type.stringValue),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.data(named: "type", value: product.type.stringValue),
.data(named: "type", value: product.type.rawValue),

Comment on lines 238 to 248
var stringValue: String {
switch self {
case .library:
return "library"
case .executable:
return "executable"
case .plugin:
return "plugin"
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var stringValue: String {
switch self {
case .library:
return "library"
case .executable:
return "executable"
case .plugin:
return "plugin"
}
}

We can remove this in favour of using rawValue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey, I suppose you added a conformation to String on the ProductType?

Copy link

cla-bot bot commented Sep 1, 2024

Thanks so much for submitting a pull request. We appreciate your contribution!

We require all contributors to sign our Contributor License Agreement (CLA) before we can merge any code, and we don't currently have a record of a signed CLA for @supersonicbyte. Please email [email protected] and let us know you need to sign a CLA, and we'll get the template over to you.

@supersonicbyte
Copy link
Collaborator Author

supersonicbyte commented Sep 1, 2024

@finestructure Ok, updated! Please take a look when you find the time. It should work on your end, I was missing one variable in the front end (I probably forgot to commit it) hence the blank you were getting.

Copy link
Member

@daveverwer daveverwer left a comment

Choose a reason for hiding this comment

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

This is great, thanks so much @supersonicbyte! Thank you for your contribution.

I had some thoughts on how we handle executables, but as discussed yesterday, that would be a separate PR. I'll also make a new issue for it, so this can be considered to fix and close #3297.

@daveverwer
Copy link
Member

@cla-bot recheck please

@daveverwer
Copy link
Member

@cla-bot check

Copy link

cla-bot bot commented Sep 1, 2024

The cla-bot has been summoned, and re-checked this pull request!

@cla-bot cla-bot bot added the cla-signed label Sep 1, 2024
@finestructure finestructure mentioned this pull request Sep 2, 2024
@finestructure
Copy link
Member

I've opened a superseding PR #3351 to run the CI, update the snapshots, and fix a linting issue.

This one here should automatically be closed once #3351 is merged.

@finestructure finestructure merged commit acfa65c into SwiftPackageIndex:main Sep 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deal with non-library products better in "Use this Package"
3 participants