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

Restructure project to test SourceGen code generation #2

Merged
merged 12 commits into from Sep 21, 2021

Conversation

odmir
Copy link
Contributor

@odmir odmir commented Sep 18, 2021

I was working on adding source generation but found out it was already contributed in the previous PR, which is pretty cool because now anyone can more easily experiment with their own distributed actors. Since I was already working on this I thought I would at least contribute:

  • a restructuring of the project to make Analysis and SourceGen testable
  • adjust SourceGen to not depend on a file URL, returning the generated code instead as an array of buckets (as a String), so that tests don't have to generate files and read them back
  • main.swift now does the writing of the buckets generated by SourceGen into files by itself
  • add a test for SourceGen
  • adjust test and SourceGen to generate a more consistent formatting of the generated code

In order to disturb the project as little as possible I restructured the project this way:

  • create a target and a library product by the name of "FishyActorsCore"
  • move Analysis and SourceGen into the new module
  • make the important methods from Analysis and SourceGen public

I am fine with adjusting the PR if there is a more preferable restructuring of the project to make it more testable.

Note: I know this is meant as "just" a small example project, but I think making it more real and functional is a good way to demonstrate the idea and allow people to more easily experiment with their own ideas.

@ktoso
Copy link
Member

ktoso commented Sep 20, 2021

Hmm... the "restructuring" I don't think is necessary though, right?

Because we can test executable targets now: apple/swift-package-manager#3316 Allow unit test targets to import and link executable targets #3316. So we can just add a test target for FishyActorsGeneratorTests/ and add the test in there. Currently the source gen test being in the "Transport" tests is a bit wrong; "transport" is the runtime library.

I ended up breaking my dev env today so can't check right now but I'll check in a bit.

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Thanks, happy to take this in but it needs some polish -- it ended up a bit more confusing than the previous incomplete thing :)

I commented inline, please let me know if anything's unclear.
Thanks for spending your time on this!

FishyTransport/Package.swift Outdated Show resolved Hide resolved
FishyTransport/Sources/FishyActorsCore/Analysis.swift Outdated Show resolved Hide resolved
FishyTransport/Sources/FishyActorsCore/Analysis.swift Outdated Show resolved Hide resolved
FishyTransport/Sources/FishyActorsCore/SourceGen.swift Outdated Show resolved Hide resolved

private let sourceDirectory: String
private let verbose: Bool

var decls: [DistributedActorDecl] = []
public var decls: [DistributedActorDecl] = []
Copy link
Member

Choose a reason for hiding this comment

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

please don't make vars public like that;

There should rather be a function that allows returns this -> the run() could return this or there could be another getAnalysedDecls or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed public here since it wasn't needed anymore. Since Analysis can't be accessed from other modules do you think it's still valuable to add a method to access it? I like the idea of making decls private and having run() return it, do you think that's a better approach?

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of making decls private and having run() return it, do you think that's a better approach?

Yeah sounds good :)

FishyTransport/Sources/FishyActorsGenerator/main.swift Outdated Show resolved Hide resolved
@odmir
Copy link
Contributor Author

odmir commented Sep 20, 2021

Hmm... the "restructuring" I don't think is necessary though, right?

Because we can test executable targets now: apple/swift-package-manager#3316 Allow unit test targets to import and link executable targets #3316. So we can just add a test target for FishyActorsGeneratorTests/ and add the test in there. Currently the source gen test being in the "Transport" tests is a bit wrong; "transport" is the runtime library.

I ended up breaking my dev env today so can't check right now but I'll check in a bit.

Ohhh that's nice, I didn't realise this was possible already, thanks for pointing it out! I will take care to revert the restructuring, it didn't feel like a very nice solution, so I'm happy it isn't needed.

undo making types and properties public and relocate `Bold` and `Reset` back to their original place
@odmir
Copy link
Contributor Author

odmir commented Sep 20, 2021

Ok, reverted it to how it was before.

and correct main.swift, `SourceGen` and unit test
@odmir
Copy link
Contributor Author

odmir commented Sep 20, 2021

Now that I understand how the buckets are supposed to be used I implemented a very naïve strategy which just assigns each decl a new bucket in incrementing fashion, which simplified main.swift and the tests.

// It will create more buckets than requested if given enough decls
if currentBucket >= buckets {
print("Warning: too many distributed actors declared, please increase requested buckets (currently \(buckets) buckets; this is a temporary limitation of FishyTransport).")
}
Copy link
Member

Choose a reason for hiding this comment

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

okey we can do this for now :)

We can do a real thing soon -- basically take the full type name, hash it, and then % with the bucket count :)

@ktoso
Copy link
Member

ktoso commented Sep 21, 2021

Great, thanks a lot @odmir !

@ktoso ktoso merged commit 6b0a0b4 into apple:main Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants