Skip to content

Convert ReactPropForShadowNodeSpecTest to Kotlin #39284

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

Closed
wants to merge 1 commit into from

Conversation

IzuEneh
Copy link
Contributor

@IzuEneh IzuEneh commented Sep 5, 2023

Summary:

This PR converts ReactPropForShadowNodeSpecTest to Kotlin. #38825

Changelog:

[INTERNAL] [CHANGED] - Kotlinify ReactPropForShadowNodeSpecTest.java

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

Test Plan:

./gradlew :packages:react-native:ReactAndroid:test must pass

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 5, 2023
Copy link
Contributor

@rshest rshest left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this!

Just a couple of nitpick comments :)


companion object {
private class BaseViewManager(
private val mShadowNodeClass: Class<out ReactShadowNode<ReactShadowNodeImpl>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Using Hungarian notation is not idiomatic for Kotlin, could you please name it shadowNodeClass instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@Ignore
class ReactPropForShadowNodeSpecTest {

companion object {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please move the companion object to the end of the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved

companion object {
private class BaseViewManager(
private val mShadowNodeClass: Class<out ReactShadowNode<ReactShadowNodeImpl>>
): ViewManager<View?,ReactShadowNode<*>>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

View could probably be non-nullable here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

object : ReactShadowNodeImpl() {
@ReactPropGroup(names = ["prop1", "prop2"])
fun setterWithNoIndexParam(value: Float, boolean: Boolean) {}
}::class.java
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ::class.java here vs .javaClass in prior clauses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Silly mistake on my end. This has been updated

@facebook-github-bot
Copy link
Contributor

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

@IzuEneh
Copy link
Contributor Author

IzuEneh commented Sep 6, 2023

Thanks a lot for working on this!

Just a couple of nitpick comments :)

Hey thanks for the quick review @rshest ! I actually created this PR because I ran into a roadblock. The issue (#38825) asks us to

  1. Fix the tests and then
  2. kotlinify them

I was able to kotlinify the test but i am stuck on making it pass hence why it is still ignored.

I run into an Unsatisfied link error caused by the yoga dependency
image
I didn't want to make any changes to the gradle files so I was wandering if anyone knew the next steps to remedy this.

@IzuEneh IzuEneh changed the title Replace java test with kotlin test Convert ReactPropForShadowNodeSpecTest to Kotlin Sep 6, 2023
@rshest
Copy link
Contributor

rshest commented Sep 6, 2023

@IzuEneh I'll check it out, but otherwise we could take fixing as a separate follow-up.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 6, 2023
@facebook-github-bot
Copy link
Contributor

@rshest merged this pull request in 8973978.

rshest added a commit to rshest/react-native that referenced this pull request Sep 6, 2023
Summary:
## Changelog:
[Internal] -

Follow up to facebook#39284 (D48960815), which landed and broke one of the build clauses on CircleCI tests (treating unused parameters as errors, even though they are expected to be unused).

Differential Revision: D49010472
rshest added a commit to rshest/react-native that referenced this pull request Sep 6, 2023
Summary:

## Changelog:
[Internal] -

Follow up to facebook#39284 (D48960815), which landed and broke one of the build clauses on CircleCI tests (treating unused parameters as errors, even though they are expected to be unused).

Reviewed By: hoxyq

Differential Revision: D49010472
facebook-github-bot pushed a commit that referenced this pull request Sep 6, 2023
Summary:
Pull Request resolved: #39311

## Changelog:
[Internal] -

Follow up to #39284 (D48960815), which landed and broke one of the build clauses on CircleCI tests (treating unused parameters as errors, even though they are expected to be unused).

Reviewed By: hoxyq

Differential Revision: D49010472

fbshipit-source-id: 469bf3a9923b85e465d4574e69e9372c16fbc125
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Sep 6, 2023
Summary:
Pull Request resolved: facebook#39311

## Changelog:
[Internal] -

Follow up to facebook#39284 (D48960815), which landed and broke one of the build clauses on CircleCI tests (treating unused parameters as errors, even though they are expected to be unused).

Reviewed By: hoxyq

Differential Revision: D49010472

fbshipit-source-id: 469bf3a9923b85e465d4574e69e9372c16fbc125
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants