[194] Change return True/False pattern to try/catch pattern #923
Conversation
- Update multiaddr dependency from 3ea7f866fda9268ee92506edf9d8e975274bf941 to db8124e2321f316d3b7d2733c7df11d6ad9c03e6 - This fixes the build failure caused by missing py-cid branch 'acul71/new-setup' - Resolves Read the Docs build failure in PR libp2p#923
There was a problem hiding this comment.
@SuchitraSwain You can take a look at other methods which is still using the True / False return value and change it to responce / exception.
| except HostException: | ||
| raise | ||
| except Exception as e: | ||
| raise HostException(f"Failed to set stream handler: {e}") from e |
There was a problem hiding this comment.
@SuchitraSwain Thank you for adding the try-catch block and input validation to this function. However, I believe this PR may not be addressing the core issue described in #194.
Issue #194 is specifically about "changing return True/False pattern to try/catch pattern," but the True/False pattern has already been removed from py-libp2p in previous commits.
For reference, the earlier implementation at this commit did use the True/False return pattern, but the current codebase has already migrated away from this pattern.
|
@SuchitraSwain Have you tried running all the tests locally? Are they all passing? |
YES I have tested it locally, everything passes |
|
@SuchitraSwain resolve the merge conflicts. |
|
@seetadev kindly run the CI/CD pipeline. |
|
@sumanjeet0012 : Ran the CI/Cd pipeline. Waiting for test results. |
|
@SuchitraSwain : 2 test cases are failing. Please collaborate with @sumanjeet0012 and @acul71 to get them fixed and arrive at a good conclusion on this PR. |
|
@seetadev could you please re-run the ci/cd again |
|
@seetadev The PR is ready |
|
@SuchitraSwain : Thank you so much Suchitra for considering our feedback points. Appreciate it. Will do a final review today. Please add a test suite and a newsfragment file too. CCing @sumanjeet0012, who will help you complete these aspects in the PR. |
There was a problem hiding this comment.
@SuchitraSwain The PR looks good to me and is ready to go from my side. I have just left a minor piece of feedback for your consideration.
Once it is addressed, we will run the CI/CD pipeline and proceed with merging this PR.
What was wrong?
Issue #194
How was it fixed?
Replaced return value pattern with try/catch pattern:
Summary of approach.
To-Do
Cute Animal Picture