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 : Return correct error code for esp_websocket_client_send_with_opcode (IDFGH-11291) #393

Closed
wants to merge 2 commits into from

Conversation

sjames
Copy link
Contributor

@sjames sjames commented Oct 23, 2023

This function esp_websocket_client_send_with_opcode was incorrectly sending ESP_FAIL even on success. Fixing this.

Fixes issue: #392

This function esp_websocket_client_send_with_opcode was incorrectly
sending ESP_FAIL even on success.  Fixing this.
@CLAassistant
Copy link

CLAassistant commented Oct 23, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title fix : Return correct error code for esp_websocket_client_send_with_opcode fix : Return correct error code for esp_websocket_client_send_with_opcode (IDFGH-11291) Oct 23, 2023
Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@gabsuren
Copy link
Contributor

@sjames thank you for the contribution.
Nitpick, I would like to suggest rename of int error = ESP_OK to int ret = ESP_OK, as in a good case it is ESP_OK which is not error.
And also need to modify commit message to fix(websocket): Return error code correctly so it can pass pre-commit hook check

Thanks again

@gabsuren
Copy link
Contributor

@sjames thank you for the update! The changes look good to me.
But as you see now there are two commits now
Screen Shot 2023-10-24 at 11 27 42 AM

The CI will still fail if we merge it like that. You can merge those to commit to one, by

  1. git rebase -i HEAD~2
  2. replace pick with squash for the second commit
  3. Edit the Commit Message:
  4. Finalize the Rebase and push changes by --force (git push --f)

If you face any issues, I can cherry-pick your changes(keeping the history/metadata) and push it in separate PR.
Please let me know in case any help needed.

Thanks
Suren

@sjames
Copy link
Contributor Author

sjames commented Oct 24, 2023

@sjames thank you for the update! The changes look good to me. But as you see now there are two commits now Screen Shot 2023-10-24 at 11 27 42 AM

The CI will still fail if we merge it like that. You can merge those to commit to one, by

1. `git rebase -i HEAD~2`

2. replace pick with squash for the second commit

3. Edit the Commit Message:

4. Finalize the Rebase and push changes by --force  (`git push --f`)

If you face any issues, I can cherry-pick your changes(keeping the history/metadata) and push it in separate PR. Please let me know in case any help needed.

Thanks Suren

I think I followed the instructions and pushed the changes. If this still doesn't work, I would really appreciate your cherry-picking offer!

Thanks,
Sojan

@gabsuren
Copy link
Contributor

Created #401 based on this PR

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.

5 participants