Skip to content

Conversation

@findmyhappy
Copy link

@findmyhappy findmyhappy commented Jun 4, 2025

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

fix function name in comment

Copy link
Contributor

@outSH outSH left a comment

Choose a reason for hiding this comment

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

Thanks! I've left a comment in code.

Also, please update the commit/PR message to fix(fabric-interop-cc): fix function name in comment - we use parentheses to specify component that is being changed, and it's more of a fix than a chore work.

And, if possible, try to include short description of the changes in the commit message.

@findmyhappy findmyhappy changed the title chore: fix function name in comment fix(fabric-interop-cc): fix function name in comment Jun 5, 2025
@findmyhappy
Copy link
Author

Please review again.

Copy link
Contributor

@outSH outSH left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@findmyhappy
Copy link
Author

LGTM, thanks!

My pleasure.

@sandeepnRES
Copy link
Contributor

Hi @findmyhappy , thanks for the PR, but one question, Is there a need for these changes? I don't see how existing comment is a problem.

@findmyhappy
Copy link
Author

Hi @findmyhappy , thanks for the PR, but one question, Is there a need for these changes? I don't see how existing comment is a problem.

Thank you for your response. I noticed that the first word in the comments of the two methods was inconsistent with the method names, which led me to make modifications. The other maintainer also gave me additional suggestions.

You are more familiar with this, and I am willing to listen to and respect your advice.

Thanks again.

@sandeepnRES
Copy link
Contributor

sandeepnRES commented Jul 8, 2025

Hi @findmyhappy , thanks for the PR, but one question, Is there a need for these changes? I don't see how existing comment is a problem.

Thank you for your response. I noticed that the first word in the comments of the two methods was inconsistent with the method names, which led me to make modifications. The other maintainer also gave me additional suggestions.

You are more familiar with this, and I am willing to listen to and respect your advice.

Thanks again.

Yeah you are right function name is inconsistent. Can you add correct the function name in the comment description, just to be consistent with comments at other places.
Thank you.

@RafaelAPB
Copy link
Contributor

@outSH @sandeepnRES are we good to merge this?

@outSH
Copy link
Contributor

outSH commented Jul 28, 2025

@RafaelAPB Fine by me, but approval from Sandeep is more important here :)

@sandeepnRES
Copy link
Contributor

sandeepnRES commented Aug 5, 2025

Hi @findmyhappy , thanks for the PR, but one question, Is there a need for these changes? I don't see how existing comment is a problem.

Thank you for your response. I noticed that the first word in the comments of the two methods was inconsistent with the method names, which led me to make modifications. The other maintainer also gave me additional suggestions.
You are more familiar with this, and I am willing to listen to and respect your advice.
Thanks again.

Yeah you are right function name is inconsistent. Can you add correct the function name in the comment description, just to be consistent with comments at other places. Thank you.

I wanted this to be addressed before merging.
Also I see that there is a merge commit created into this. Can you please remove it and merge with rebase only to have linear history?
@findmyhappy @RafaelAPB Thanks.

@sandeepnRES
Copy link
Contributor

sandeepnRES commented Aug 26, 2025

@findmyhappy Closing this PR, as it has merge commit and thus can not be merged, and it seems you are busy. So please create a fresh PR (when you are back) where there is only one commit and please address my comment above.

@findmyhappy
Copy link
Author

findmyhappy commented Oct 28, 2025

@findmyhappy Closing this PR, as it has merge commit and thus can not be merged, and it seems you are busy. So please create a fresh PR (when you are back) where there is only one commit and please address my comment above.

@sandeepnRES Sorry for coming late to handle this. I have been busy with school evaluations for the past two months.😂

Please review again. #4052

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.

4 participants