Automattic / Gridicons-iOS

Gridicons is a tiny framework which generates Gridicon images at any resolution.
GNU General Public License v2.0
25 stars 12 forks source link

Add Package.swift #71

Closed crazytonyli closed 1 year ago

crazytonyli commented 1 year ago

Not just adding a Package.swift file, this PR also proposes a structure that potentially can be shared among all our shared libraries.

Development

During development, we should treat the library as a Swift package. There are two ways to develop the Swift package in Xcode:

Both approaches work, they don't conflict with each other. I don't see there is any need of recommending one over the other. But we may want to add steps in CI to ensure the demo/example app continues to build.

Distribution

The library can be imported as a Swift package and a CocoaPods pod. But since we no longer use the library's podspec file during development, we could easily overlook CocoaPods integration. One way to prevent this issue is adding "test spec" to the podspec so that the pod lib lint step in CI not only validates if the podspec can be imported as a pod but also tests its functionality—kinda like how you can build and test targets in a Swift Package.

I want to highlight one thing. Since CocoaPods and SPM package the library differently, especially on the resource bundles, we'll need to make sure there is test coverage around loading resource bundles.

Summary

To summarize all above, here are list of things that we should do in our libraries.

(*) xcodebuild has this very strange behavior when dealing with Package.swift. To make the command run tests in the Package.swift, we need to run the command in the directory of Package.swift, without passing -workspace nor -project option. Suppose there is an xcworkspace or xcodeproject in the Package.swift directory, they will use them instead of the Package.swift file—there is no equivalent of -workspace MyApp.workspace for Swift Package.


crazytonyli commented 1 year ago

@mokagio I actually changed my mind regarding the Package.resolved file. We probably should check in this file, like how we check in the Podfile.lock file.

crazytonyli commented 1 year ago

This repo doesn't follow the recommended (?) SPM folder structure

My plan for now is keeping the existing folder structure, for two reasons

  1. Package.swift DSL should be flexible enough to support any random folder structure.
  2. Not renaming folders and files makes git blame easier.
mokagio commented 1 year ago

This repo doesn't follow the recommended (?) SPM folder structure

My plan for now is keeping the existing folder structure, for two reasons

  1. Package.swift DSL should be flexible enough to support any random folder structure.
  2. Not renaming folders and files makes git blame easier.

@crazytonyli fair enough. I don't feel strongly about the file names. Well, actually... A part of me really wishes we had Sources/ Tests/ Demo/ across repos because it would look neat and homogeneous, but I'm not aware of any practical benefit that would make development easier if we did that 😄

Doing a blanket rename is a straightforward operation that we can do anytime in the future if a good reason for it comes up, so no worries 👍


I was curious about the git blame behavior and pushed a branch with renames and checked the blame on GitHub. It seems to follow renames alright. Example, I renamed the whole Gridicons/Gridicons/ folder to Sources/ and checked one file:

image

AliSoftware commented 1 year ago

I was curious about the git blame behavior and pushed a branch with renames and checked the blame on GitHub. It seems to follow renames alright. Example, I renamed the whole Gridicons/Gridicons/ folder to Sources/ and checked one file:

Yes, git blame follow full-file renames (i.e. if you just renamed a file in a commit without also changing some of its content at the same time) pretty well automatically. If you made both a rename and a content change, you can still get git blame follow those by using -C / -M options, that just makes the blame command a bit slower.

The origin of lines is automatically followed across whole-file renames (currently there is no option to turn the rename-following off). To follow lines moved from one file to another, or to follow lines that were copied and pasted from another file, etc., see the -C and -M options. — Source: https://www.git-scm.com/docs/git-blame

So if we wanted to reorganize the files to follow SPM filesystem structure conventions, that wouldn't cause any trouble with git blame at least.

crazytonyli commented 1 year ago

@AliSoftware I can't remember the exact issue, but I had troubles with git blame option, maybe I wasn't using them correctly.

For easier PR review, I can create a follow up PR to follow SPM's convention, what do you all think?

AliSoftware commented 1 year ago

Yeah if you want to try that doing so in a separate PR would be a good way to go imho.

I wonder if it makes a difference to git blame if you used git mv (vs mv + git add) to rename/move the files… 🤔 I don't think so but to worth checking?

mokagio commented 1 year ago

For easier PR review, I can create a follow up PR to follow SPM's convention, what do you all think?

Sounds good to me. I am not too worried about adhering to the Sources/ Tests/ structure. It can happen later, if at all.