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.

Sign up for GitHub

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

Jump to bottom

Use Rust FFI #3: Consumer #280

Closed
wants to merge 50 commits into from
Closed

Use Rust FFI #3: Consumer #280

wants to merge 50 commits into from

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Dec 26, 2022 •
edited

This is 3nd pull request in a series of "small" pull requests I split from #210

This pull request depend on #279, please review it first.

tienvx
tienvx commented Dec 26, 2022
View reviewed changes
UPGRADE-9.0.md Outdated Show resolved Hide resolved
tienvx
tienvx commented Dec 27, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@tienvx tienvx mentioned this pull request Dec 28, 2022
Closed
@cfmack
Copy link
Collaborator

cfmack commented Jan 11, 2023

Can you merge in the current head of master into your branch?

@tienvx tienvx mentioned this pull request Mar 2, 2023
Closed
This was referenced Mar 2, 2023
Closed
Closed
@YOU54F
Copy link
Member

YOU54F commented May 11, 2023

You've done here massive work and I really appreciate it. I hope you won't get my reviews as pain in the ass :) If you want you always can ignore them :)

I am sure @tienvx appreciates your input @Greg0! I certainly do, I am learning stuff watching you both at work via my GH notifications :)

@YOU54F
Copy link
Member

YOU54F commented May 11, 2023 •
edited

I hope you won't get my reviews as pain in the ass :)

then proceeds to provide an awesome review - you remind me of an engineer I used to work with, that is exactly what he would say 😅

@tienvx tienvx mentioned this pull request May 12, 2023
Merged
@Greg0
Copy link
Contributor

Greg0 commented May 18, 2023

Greeting after a slight absence. @tienvx let us know when you consider the work done. I'll look at the whole change again.

Any smaller cosmetics still can be done in separate PR later.

@tienvx
Copy link
Contributor Author

tienvx commented May 18, 2023

it's done, please review again. I also created #313 as an effort to make this PR smaller so it's easier to review

Greg0
Greg0 approved these changes May 19, 2023
View reviewed changes
Copy link
Contributor

@Greg0 Greg0 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Greg0
Copy link
Contributor

Greg0 commented May 19, 2023

@cfmack Do you have anything to add here?

This was referenced Jul 10, 2023
Closed
Closed
@YOU54F
Copy link
Member

YOU54F commented Jul 24, 2023

merged in #323

02b7fc3

@Greg0
Copy link
Contributor

Greg0 commented Jul 24, 2023

Great! I will look to the next PR then

@YOU54F
Copy link
Member

YOU54F commented Jul 24, 2023

hey @Greg0

all the changes of Tiens PR's are consolidated and available for testing as per the instructions at the top of this pinned issue

#262

hope that helps!

thanks for being involved!

@tienvx
Copy link
Contributor Author

tienvx commented Jul 25, 2023

Thanks @YOU54F . I will closes my PRs.

@tienvx tienvx closed this Jul 25, 2023
@YOU54F YOU54F mentioned this pull request Jul 25, 2023
Merged
12 tasks
@tienvx tienvx deleted the use-rust-ffi-consumer branch July 31, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfmack cfmack cfmack left review comments

@Greg0 Greg0 Greg0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tienvx @cfmack @Greg0 @YOU54F