Skip to content
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

Added a databases category to the Packages page #622

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

daveverwer
Copy link
Collaborator

As suggested in this Swift forums thread this PR adds a “Database access” package list to the Packages page.

There is more discussion in the original thread.

Screenshot 2024-04-01 at 12 48 21@2x

Copy link
Collaborator

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Query around the casing but LGTM

@@ -488,9 +488,84 @@ categories:
to log details to the console (and optionally a file) with additional information
such as date, function name, and line number.
owner: Dave Wood
- name: Database access
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit - WDYT

Suggested change
- name: Database access
- name: Database Access

Copy link
Collaborator

@kaishin kaishin left a comment

Choose a reason for hiding this comment

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

Nice one!

I am a bit unsure about the "Access" part of the category title as some of these packages aren't necessarily focused on interfacing with databases.

Would something like "Databases & Persistence" be acceptable or is it a mouthful? Otherwise just "Databases" would cast a wider net in my view.

platform_compatibility:
- Apple
- Linux
platform_compatibility_tooltip: Apple (macOS) and Linux
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is tangential to the PR since it's also the case on the SPI page, but why is iOS/tvOS not listed for GRDB even though they are supported by the library?

Copy link

@groue groue Apr 3, 2024

Choose a reason for hiding this comment

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

Good question @kaishin 👍 I think it's because the GRDB repository comes with 2 Xcode projects - this trips up SPI: https://swiftpackageindex.com/groue/GRDB.swift/builds

@daveverwer, do you think you can manually update the platform_compatibility_tooltip with iOS, macOS, tvOS, visionOS, watchOS, and Linux, even if the PR changes have already been approved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately manually updating this is not going to be possible. The data files that drive these pages are automatically generated from SPI data and so every time that tool gets run (which happens monthly) it would overwrite any manual changes.

I'd much rather get this fixed on the SPI side, so that it's not only correct here but also on SPI. I vaguely remember this coming up before, did we already go through troubleshooting the builds? If you want to open a new "Incorrect package data" issue and we can look at what's causing the failures. We do have some options you can specify in your .spi.yml that are able to solve common problems with builds.

Copy link

@groue groue Apr 3, 2024

Choose a reason for hiding this comment

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

Sure, Dave. It was SwiftPackageIndex/SwiftPackageIndex-Server#1890

I don't know if SwiftPackageIndex/SwiftPackageIndex-Server#1915 was settled or not, and if I should do something to my .spi.yml.

Copy link

@groue groue Apr 9, 2024

Choose a reason for hiding this comment

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

@daveverwer, do you think I have something else to do on my side so that the package metadata is fixed, and users don't think GRDB only works on a subset of the actually supported platforms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apologies for not getting back to you on this. @finestructure and I chatted about the compatibility issue and while we do still plan to add explicit support for multiple projects, unfortunately, the change needed touches multiple parts of our application and isn't something we can rush through for this.

There is a potential workaround in your original issue. We understand that adopting a workaround like this is not ideal, and of course, we will get to SwiftPackageIndex/SwiftPackageIndex-Server#1915 as soon as possible, but I can't say when as we have not yet had reports of any other packages in need of this feature.

I’m sorry I don’t have better news!

Copy link

@groue groue Apr 10, 2024

Choose a reason for hiding this comment

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

I can't move the extra Xcode project because this would be breaking change for users.

I can't gather all targets in a single Xcode project (in a future major release) because one of the projects is used for advances use cases (custom SQLite builds) and depends on a submodule that not everybody clones. When all targets are gathered in a single project, the missing submodule creates an awful ton of errors and warnings in Xcode - not something I want users to see.

I can't say when as we have not yet had reports of any other packages in need of this feature.

Well understood, @daveverwer, and thank you for your investigation of the topic. I can only appeal to your generosity, or the itch created by this wrong meta-data.

If GRDB is well-appreciated, it is also because it goes far in servicing the various needs of its users - hence the two Xcode projects. My priority is and will remain GRDB users.

Copy link
Collaborator

@dempseyatgithub dempseyatgithub left a comment

Choose a reason for hiding this comment

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

This LGTM!

@federicobucchi
Copy link
Member

@swift-ci please test

@daveverwer daveverwer marked this pull request as draft April 4, 2024 17:10
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.

None yet

6 participants