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

Changes for Complete coroutine. #218

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

favalos
Copy link
Contributor

@favalos favalos commented Jan 25, 2024

WIP for #166.

@guergabo I did the changes for this issue.

I marked this as Draft because I am getting a couple of test failures, if you have any advice it would be great.

Any comment or suggestions are welcome.

Thanks!

@dfarr
Copy link
Member

dfarr commented Jan 26, 2024

Thank you for the PR again @favalos!

I think the reason you are seeing test failures is because you re-ordered the states here. So now the constants map to:

const (
	Pending State = 1 << iota
	Resolved // 2
	Rejected // 4
	Canceled // 8
	Timedout // 16
	Invalid  // 32
)

I like this ordering better but we will need to change the test cases to match for the Timedout case.

We will also need to remove Invalid from this enum and guard against ever getting to this point. Instead I recommend changing the type of State in the CompletePromiseBody to promise.State that way we can take advantage of the UnmarshalJSON function we already have and the validation would become

!body.State.In(promise.Resolved|promise.Rejected|promise.Canceled)

Copy link

codecov bot commented Jan 27, 2024

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (0773fab) 64.33% compared to head (aefc288) 63.95%.
Report is 3 commits behind head on main.

❗ Current head aefc288 differs from pull request most recent head baf23ff. Consider uploading reports for the commit baf23ff to get more accurate results

Files Patch % Lines
test/dst/generator.go 33.33% 8 Missing ⚠️
test/dst/model.go 45.45% 3 Missing and 3 partials ⚠️
internal/app/coroutines/completePromise.go 88.88% 3 Missing ⚠️
internal/app/subsystems/api/http/promise.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
- Coverage   64.33%   63.95%   -0.38%     
==========================================
  Files          94       92       -2     
  Lines       10141     9652     -489     
==========================================
- Hits         6524     6173     -351     
+ Misses       3164     3042     -122     
+ Partials      453      437      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@favalos
Copy link
Contributor Author

favalos commented Jan 27, 2024

Hey @dfarr,

Thanks for the hint, I re-ordered the Enum and that did the trick.

I fixed another couple of errors that I found and did the changes related to the recommendation mentioned above.

Thank you!

@favalos favalos marked this pull request as ready for review January 27, 2024 05:20
@dfarr
Copy link
Member

dfarr commented Jan 29, 2024

Looks great @favalos! The only other change I would recommend would be consolidating the t_api Kind to a single kind for these requests called CompletePromise. This is a lot of copypasta work so let me know if you want me to do that and I can add a quick commit.

@favalos
Copy link
Contributor Author

favalos commented Jan 29, 2024

I can work on that tonight, no worries!

Thanks!

@favalos
Copy link
Contributor Author

favalos commented Jan 30, 2024

@dfarr

Done! Let me know any other suggestions.

Thanks!

@dfarr
Copy link
Member

dfarr commented Jan 30, 2024

Awesome thank you so much @favalos 🎉🎉 🎉

We just need one return statement and I will merge this PR :)

@dfarr dfarr merged commit 268c588 into resonatehq:main Jan 30, 2024
5 checks passed
@favalos favalos deleted the complete-coroutine-#166 branch January 30, 2024 20:10
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.

2 participants