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

Resolve multi-preview annotations in other modules #69

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

trevor-e
Copy link
Contributor

@trevor-e trevor-e commented Sep 27, 2023

Resolves multi-previews that are defined in other modules by:

  1. Finds all symbols with an annotation in the current module
  2. Of those annotations, finds those that have a @Preview annotation and assumes they are used for multi-preview
  3. Loads the class declaration for these previews by the qualified name, which importantly seems to work fine across modules
  4. Finds compose functions for these annotations

Produces the following sample output:
Screenshot 2023-09-27 at 3 12 41 PM

// We can assume these are multi-preview annotations.
return annotationClassDecls
.filter {
it.annotations.any { annotation ->
Copy link
Member

Choose a reason for hiding this comment

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

Will this get annotations that are transitively added to this annotation? Ex:

@Preview(...)
annotation class Class1

@Class1
annotation class Class2

Will it pick up Class2 as having a preview annotation? If not can we do this in a loop building up what annotations are eligible to make a preview?

Copy link
Contributor

Choose a reason for hiding this comment

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

We had discussed v1 of this only supporting a single level of Multipreview nesting (and checked with clients that that was ok), so that means we're only intending to cover your first example here.

But, if it's easy enough to add, we might as well add support for all potential cases here.

}
}
}
.filter { it.classKind == ClassKind.ANNOTATION_CLASS }
Copy link
Contributor

Choose a reason for hiding this comment

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

My only potential nit here would be to move this up above .filter { it.annotations.count() > 0 }, that way we're only working with annotation class declarations at that point, and don't risk running annotation.count() and getClassDeclarationByName on classes that are not annotation classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think that's possible since it is a KSDeclaration for the sequence of symbols with annotations, we haven't mapped to the annotation class declarations yet.

I think the filter is actually redundant since we are flatmap'ing to the annotation class declaration, so it should only be a sequence of annotation classes at this point, but it does make it a little more readable so I'll leave it in.

@@ -25,15 +26,17 @@ fun TextRowWithIcon(

@Preview
@FontScalePreviews
@LocalePreviews
Copy link
Contributor

Choose a reason for hiding this comment

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

Ran locally and this outputted 5 previews, matching what I expected. Awesome job!

Might be good for us to add a test case in snapshots-processor-test for this, but inherently that's a bit tough as it currently relies on a single /input directory producing outputs. Maybe adding an input2 or submodule containing a multipreview with an import/usage in input would produce a valid test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea testing this has proven to be difficult, I'll keep trying some things.

@trevor-e trevor-e changed the title [WIP] Resolve multi-preview annotations in other modules Resolve multi-preview annotations in other modules Sep 27, 2023
Copy link
Contributor

rbro112 commented Sep 27, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@rbro112 rbro112 mentioned this pull request Sep 27, 2023
@trevor-e trevor-e merged commit 5bce254 into main Sep 27, 2023
5 checks passed
@trevor-e trevor-e deleted the telkins/find-module-annotations branch October 18, 2023 17:07
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.

3 participants