Skip to content

Conversation

@lyy-pineapple
Copy link
Contributor

What changes were proposed in this pull request?

In ResourceProfileManager, function calls should occur after variable declarations

Why are the changes needed?

As the title suggests, in ResourceProfileManager, function calls should be made after variable declarations. When determining isSupport, all variables are uninitialized, with booleans defaulting to false and objects to null. While the end result is correct, the evaluation process is abnormal.
image

Does this PR introduce any user-facing change?

No

How was this patch tested?

through exists uts

Was this patch authored or co-authored using generative AI tooling?

No

Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

Can you add a test to ensure this does not recur in future ?
+CC @tgravescs

@tgravescs
Copy link
Contributor

change looks fine to me, thanks for fixing

@lyy-pineapple
Copy link
Contributor Author

Can you add a test to ensure this does not recur in future ? +CC @tgravescs

Can you add a test to ensure this does not recur in future ? +CC @tgravescs

Adding tests can be challenging because all processes are within the initialization. To avoid this issue, we can add assert(master != null) in the function. Alternatively, consider changing the following variables to lazy val to ensure they are initialized before usage.I have chosen to include an assert statement here.

@lyy-pineapple
Copy link
Contributor Author

@tgravescs @mridulm thanks for review, do you have any further comments?

@mridulm mridulm closed this in 38fc127 Jan 18, 2024
@mridulm
Copy link
Contributor

mridulm commented Jan 18, 2024

Merged to master.
Thanks for fixing this @lyy-pineapple !
Thanks for the review @tgravescs :-)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants