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

Rename constructors to try_new #3199

Merged

Conversation

p-andhika
Copy link
Contributor

Description

This PR try to tackle issue #3179.

Changes

  • Rename all constructors that return Result to try_new.

How to test

Existing unit test.

@p-andhika p-andhika requested a review from a team as a code owner December 31, 2024 14:12
Copy link

github-actions bot commented Dec 31, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@p-andhika
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@p-andhika
Copy link
Contributor Author

recheck

@sunce86
Copy link
Contributor

sunce86 commented Dec 31, 2024

Thanks for the PR.

Next step is to make our tests pass. Can you check all the failing CI runs and look for errors and fix them?

@p-andhika
Copy link
Contributor Author

p-andhika commented Dec 31, 2024

I've fix the ci jobs for lint and doc-test. But I'm not sure about test-forked-node job.

@squadgazzz
Copy link
Contributor

But I'm not sure about test-forked-node job.

It is not possible to run this test from a forked repo for security reasons. We should probably add a reminder about that to all the external submissions.

@p-andhika
Copy link
Contributor Author

So, what should I do from my side?

@MartinquaXD
Copy link
Contributor

So, what should I do from my side?

There is no need to do anything from your side. Thank you for your contribution.

@MartinquaXD MartinquaXD merged commit 0ff75fa into cowprotocol:main Jan 2, 2025
10 of 11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants