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 PropDefault values extraction for Kotlin #371

Closed

Conversation

pavlospt
Copy link
Contributor

@pavlospt pavlospt commented May 24, 2018

  • There was an issue with the current implementation if a user tried to use that same annotation prefixed with get.

e.g.: @get:PropDefault

The solution is to identify which is the case and fallback to the respective method that holds the @PropDefault annotation on the equivalent Java file.

With @get:

   @PropDefault(
      resType = ResType.DIMEN_SIZE,
      resId = 2131034112
   )
   @Nullable
   public final Float getImageAspectRatio() {
      return imageAspectRatio;
   }

Without @get:

   /** @deprecated */
   // $FF: synthetic method
   @PropDefault(
      resType = ResType.DIMEN_SIZE,
      resId = 2131034112
   )
   public static void imageAspectRatio$annotations() {
   }

* There was an issue with the current implementation if a user
tried to use that same annotation prefoxed with `get`.

e.g.: `@get:PropDefault`

Solution is to identify which is the case and fallback to the respective
method that holds the `@PropDefault` annotation on the equivalent Java file.
@passy
Copy link
Member

passy commented May 24, 2018

Great stuff! As this is getting more complex, would it be possible to add a test case for this in PropDefaultsExtractorTest? I'm not quite sure if this would just be a matter of essentially copying over the two examples you have there or if there's a lot of additional setup that would be required.

@pavlospt
Copy link
Contributor Author

@passy we could copy the Java equivalent classes and use them as test cases!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@passy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@passy
Copy link
Member

passy commented May 25, 2018


Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants