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

Fix indentation in generated browser code #258

Conversation

wang-li
Copy link
Contributor

@wang-li wang-li commented Aug 23, 2022

Overview

In this PR, I fixed the indentations in ShowkaseBrowserWriter so that it does not shift the code by a double indentation every time it generates code for a preview parameter provider.

The results were asserted by looking at the generated files (the fixed indent is at line 101 in the following screenshots):

Before After

I also added the extension withDoubleIndent { } so that the indentations can safely unindent after the relevant block. This extension is based on the existing withIndent { } extension from Kotlin poet.

Finally, I wanted to add a unit test to avoid this issue to resurface but I struggled to understand how the unit tests in showkase-processor-testing work... But I'm open for some guidance! 😄

This fixes #257

Copy link
Collaborator

@vinaygaba vinaygaba left a comment

Choose a reason for hiding this comment

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

@wang-li Thank you for contributing and fixing this issue 🙏🏻

For the ShowkaseProcessor tests, here's what you will need to do:

  1. Add a new test to this file
  2. Create a folder with the same name as the test that you added in (1) and create the test Composable files in a folder called input. You will add that here - https://github.com/airbnb/Showkase/tree/master/showkase-processor-testing/src/test/resources/ShowkaseProcessorTest
  3. You can auto generated the output by setting this boolean to true and running the tests locally once - https://github.com/airbnb/Showkase/blob/master/showkase-processor-testing/src/test/java/com/airbnb/android/showkase_processor_testing/BaseProcessorTest.kt#L18
  4. Commit this test folder along with the input and output subfolders.

@wang-li wang-li force-pushed the bugfix/fix-indentation-in-generated-browser-code branch from 835acd4 to 5e77a58 Compare August 25, 2022 08:28
@wang-li wang-li force-pushed the bugfix/fix-indentation-in-generated-browser-code branch from 5e77a58 to 2037ec2 Compare August 25, 2022 08:30
@wang-li
Copy link
Contributor Author

wang-li commented Aug 25, 2022

Thank you very much for the instructions @vinaygaba! 🙏
I updated the commit, adding the relevant test and removing the previous unit tests changes in the showkase-browser-testing to reduce the noise around this PR 😉

@wang-li wang-li requested a review from vinaygaba August 25, 2022 08:34
@@ -143,6 +143,11 @@ class ShowkaseProcessorTest : BaseProcessorTest() {
"annotation")
}

@Test
fun `composable previews with parameter providers should indent properly`() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@wang-li Can you rename this test to the following since it describes it better. You will also need to rename the corresponding folder.

composable previews with multiple parameter providers should indent properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vinaygaba For sure 👍 It's done in an additional commit 😉

Copy link
Collaborator

@vinaygaba vinaygaba left a comment

Choose a reason for hiding this comment

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

One final change needed but great job in fixing this. Thank you 🥳

@wang-li
Copy link
Contributor Author

wang-li commented Aug 25, 2022

Thank you for taking the time to review my PR 🙏

@vinaygaba vinaygaba merged commit 94d583a into airbnb:master Aug 26, 2022
@wang-li wang-li deleted the bugfix/fix-indentation-in-generated-browser-code branch August 26, 2022 07:20
@wang-li
Copy link
Contributor Author

wang-li commented Aug 31, 2022

Hey @vinaygaba 👋 Would it be possible to get a new release in order to benefit this fix through a gradle dependency? 🙏

@vinaygaba
Copy link
Collaborator

@wang-li I was hoping to get one more change in before I do the next release. You can expect it tomorrow

@hhalse
Copy link

hhalse commented Dec 1, 2022

Hey @vinaygaba 👋 Will there be a new release for this fix?

@oas004 oas004 mentioned this pull request Dec 7, 2022
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.

Showkase browser generated code compilation issue
3 participants