search

stackotter / swift-macro-toolkit

A powerful toolkit for creating concise and expressive Swift macros
Apache License 2.0
246 stars 15 forks source link

Introduce modularization #5

Open maximkrouk opened 1 year ago

maximkrouk commented 1 year ago

Motivation

Some users may not want to rely on certain APIs, so it's beneficial to divide the package into multiple targets. This allows library users to selectively use features.

Impact

MacroToolkit target remains to offer a convenient option for importing all the provided targets, so no breaking changes involved and it can be a patch update if merged

Notes

stackotter commented 1 year ago

Woah that looks really clean, will definitely consider switching to that at some point! But yeah it should be a separate PR.

Example is extracted to a separate package to avoid distraction with modules

The examples and associated tests are currently acting as tests for macro toolkit, so to me it makes sense for them to be part of the same package still (it's nice to just run swift test in the repo to test the toolkit, and not having to know that the examples are actually the tests).

Some users may not want to rely on certain APIs, so it's beneficial to divide the package into multiple targets. This allows library users to selectively use features.

In your eyes what are the main benefits of allowing the features to be imported separately?

I can see why DiagnosticBuilder could make sense to be a separate target, but calling the rest MacroToolkitTypes doesn't really make sense to me. To me that sounds like it's just supporting/helper types, but it also includes extensions, destructuring, and other features.

The main reason I'm hesitant to modularise too much is that in Swift it's often tricky to know which import a type came from, because you just have to memorise it or have a really solid distinction between the different targets being imported. For example, I often find myself importing unnecessary SwiftSyntax targets because I can't tell which ones are being used and which ones aren't. And I often have to just guess which one to import to import the type that the compiler is complaining about, whereas it's pretty easy to just import MacroToolkit when you want something from the macro toolkit package.

My point is that we should modularise with care, I'm not totally opposed to it and can see some merits. I think if there were MacroToolkitCore and DiagnosticBuilder targets that would be a clear enough distinction for me to always know which target a macro toolkit type/feature is in without having to guess or use docs (as a user).

maximkrouk commented 1 year ago

The examples and associated tests are currently acting as tests for macro toolkit, so to me it makes sense for them to be part of the same package still

I think this kinda causes a bit of distraction when observing sources from users' perspective. However it's a minor thing 💁‍♂️

(it's nice to just run swift test in the repo to test the toolkit, and not having to know that the examples are actually the tests).

Can still be partially acheived using Makefile (it'll be make test then)

but calling the rest MacroToolkitTypes doesn't really make sense to me

Thats one of the reasons it's draft, I don't like this name either :D

To me that sounds like it's just supporting/helper types, but it also includes extensions, destructuring, and other features.

Destructuring probably can be extracted to a separate module as well

MacroToolkitCore

I like Core better, than Types, I also was thinking about

Core is often used for some root module, when most of modules depend on it, but it should be suitable for our case as well

stackotter commented 1 year ago

I think this kinda causes a bit of distraction when observing sources from users' perspective. However it's a minor thing 💁‍♂️

Yeah, maybe it can be moved under the assumption that the test macros will be separated at some point.

Can still be partially acheived using Makefile (it'll be make test then)

Yeah ok I'm happy with that for now.

Destructuring probably can be extracted to a separate module as well

Yeah it could be, there is definitely a strong enough distinction. But I'm not sure if there's any value in doing so.

  • MacroToolkitProxyTypes but it's too long and a bit weird
  • MacroToolkitDSL but I'm not sure if it's actually a DSL, may be a good option if it is 🌚 (when I think of DSLs I usually think about things to build something like html doc, rather than just wrapping existing types to extend them)

I agree, ProxyTypes is weird, and it's not a DSL (at the moment at least). It would have a DSL if we were introducing custom result builder (each result builder somewhat defines it's own DSL).

Core is often used for some root module, when most of modules depend on it, but it should be suitable for our case as well

Yeah, the way I'm thinking about Core is as in the core functionality that basically everyone will want to import.

maximkrouk commented 1 year ago

Yeah it could be, there is definitely a strong enough distinction. But I'm not sure if there's any value in doing so.

I don't think someone will import destructuring in isolation, so the only value I see is more clear logic separation mostly for development/contributions

Alright so for now I'll:

And there is still an issue with docc 🤔

stackotter commented 1 year ago

Alright so for now I'll:

Rename Types to Core Add Makefile

Yep sounds good 👌

And there is still an issue with docc 🤔

Hmm yeah that's an important one; what does Swift Syntax do to get around that given that it has so many targets? Ah it uses the Swift Package Index generated docs which have a target switcher at the top. Swift Macro Toolkit is already using SPI to generate and host its docs so that's probably a good enough solution for now. The SPI manifest file just needs to be updated to include the new targets (and probably not the re-exporting MacroToolkit target).

I'd be interested to know how @_reexported import ... works with documentation, do the re-exported types appear in the documentation? In that case nothing needs to change at all.

maximkrouk commented 1 year ago

@_exported imports are not shown in docs, but I think it's ok to just mention it in readme and main doc file 🤔

stackotter commented 1 year ago

@_exported imports are not shown in docs

Ah sad, fair enough. Yeah that's fine for now.