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 some comments #5680

Closed
wants to merge 3 commits into from
Closed

fix some comments #5680

wants to merge 3 commits into from

Conversation

cuishuang
Copy link

@cuishuang cuishuang commented Mar 3, 2024

Description

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@CLAassistant
Copy link

CLAassistant commented Mar 3, 2024

CLA assistant check
All committers have signed the CLA.

sdankel
sdankel previously approved these changes Mar 3, 2024
IGI-111
IGI-111 previously approved these changes Mar 7, 2024
@sdankel sdankel enabled auto-merge (squash) March 13, 2024 23:16
@sdankel sdankel disabled auto-merge April 19, 2024 05:21
@sdankel
Copy link
Member

sdankel commented Apr 19, 2024

@cuishuang I'm not sure why those tests failed. Please update your branch as it is now out of date.

@cuishuang
Copy link
Author

@cuishuang I'm not sure why those tests failed. Please update your branch as it is now out of date.

Thanks. Updated.

Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR. CI seems to be failing because you updated an error message but did not changed the test for that error message. So what you need to do make the pipeline green is changing the expected message at the related test.toml.

@cuishuang
Copy link
Author

Hi, thanks for the PR. CI seems to be failing because you updated an error message but did not changed the test for that error message. So what you need to do make the pipeline green is changing the expected message at the related test.toml.

Modified. Thanks!

kayagokalp
kayagokalp previously approved these changes Apr 29, 2024
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

Thanks!

IGI-111
IGI-111 previously approved these changes Apr 29, 2024
@IGI-111 IGI-111 enabled auto-merge (squash) April 29, 2024 15:31
@IGI-111 IGI-111 disabled auto-merge April 29, 2024 15:31
@cuishuang
Copy link
Author

Anything need I to do? Please feel free to tell me.

@IGI-111
Copy link
Contributor

IGI-111 commented May 1, 2024

Solve the merge conflicts and we should be able to merge this.

Signed-off-by: cuishuang <[email protected]>
@cuishuang
Copy link
Author

Solve the merge conflicts and we should be able to merge this.

Solved, please review again.

kayagokalp
kayagokalp previously approved these changes May 3, 2024
IGI-111
IGI-111 previously approved these changes May 6, 2024
@IGI-111
Copy link
Contributor

IGI-111 commented May 6, 2024

It's outdated again 😓

@cuishuang
Copy link
Author

It's outdated again 😓

Updated! Please review again.

xunilrj
xunilrj previously approved these changes May 8, 2024
@IGI-111
Copy link
Contributor

IGI-111 commented May 8, 2024

The HEAD is moving pretty fast so it's outdated again, please check the box that allows edits by maintainers in this PR so that we can auto update it and it can be merged.

@sdankel sdankel enabled auto-merge (squash) May 8, 2024 18:20
sdankel
sdankel previously approved these changes May 8, 2024
@sdankel sdankel disabled auto-merge May 8, 2024 18:20
@cuishuang cuishuang dismissed stale reviews from sdankel, xunilrj, kayagokalp, and IGI-111 via 02a00b4 May 9, 2024 02:28
@cuishuang cuishuang force-pushed the master branch 2 times, most recently from 55ffca4 to de5fd6c Compare May 9, 2024 02:35
Signed-off-by: cuishuang <[email protected]>
@cuishuang
Copy link
Author

cuishuang commented May 9, 2024

The HEAD is moving pretty fast so it's outdated again, please check the box that allows edits by maintainers in this PR so that we can auto update it and it can be merged.

Conflicts have been resolved and code updated.

Since I forked the project to an organization rather than to my personal account, I couldn't see the "allows edits by maintainers" option. I tried a lot and finally checked the documentation and discussions and found that from fork to organization, the project does not have this feature. Even though I created this organization and am the only member.

https://github.com/orgs/community/discussions/5634

Sorry for causing this trouble. If the problem still cannot be solved, I can close this PR, fork it to my personal account, and submit a new PR.

Waiting for your reply and decision

image

@IGI-111
Copy link
Contributor

IGI-111 commented May 9, 2024

I'm going to close this PR, please reopen one with a personal account fork and we'll get this merged.

@IGI-111 IGI-111 closed this May 9, 2024
@cuishuang
Copy link
Author

I'm going to close this PR, please reopen one with a personal account fork and we'll get this merged.

Thanks. The new pr #5989

The new pr allow edits by maintainers, Please review again.

@cuishuang cuishuang mentioned this pull request May 11, 2024
8 tasks
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.

None yet

6 participants