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

[feat] mutex + fiber + thread + io || DRAFT #14

Open
wants to merge 169 commits into
base: main
Choose a base branch
from

Conversation

ms-jpq
Copy link
Contributor

@ms-jpq ms-jpq commented Jul 8, 2022

This is just a draft, havn't wrote the tests and ensured everything works 100% yet, and I also need to add in a bunch of doc comments.

Please do leave comments & stuff for things you want to changed / updated tho <3.

Would really appreciate any points on better approaches to do things.

@ms-jpq ms-jpq force-pushed the main branch 4 times, most recently from 2f35869 to ca4732b Compare July 8, 2022 07:42
@matsadler
Copy link
Owner

Thanks so much for this!

I have basically no free time at the moment (who'd have thought having a 1 month old baby and my parents visiting from overseas would take up so much time?) so it might take a while to get back to you on this.

Normally I'd try to get all my feedback submitted at once, but that might take a very long time before you get any feedback at all, so my plan is to review individual parts as and when I can. If this turns out to be annoying, just let me know and I'll hold off on individual comments until I can do everything at once.

@ms-jpq
Copy link
Contributor Author

ms-jpq commented Jul 8, 2022

hey dw about it, i will just add in all the comments and tests and what not, and set it to ready for review.
for when ever you are ready.

btw enjoy your baby! :D :D :D must be exciting <3

@ianks
Copy link
Contributor

ianks commented Jul 10, 2022

@ms-jpq Thanks for your work here!

It would be helpful to split this PR into multiple PRs — one for IO, Fiber, etc. Would make it easier to review.

src/r_fiber.rs Outdated
Comment on lines 187 to 203
pub fn resume<A>(&self, args: A) -> Result<Value, Error>
where
A: ArgList,
{
Copy link
Owner

Choose a reason for hiding this comment

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

Can this call try_convert()? Like:

pub fn resume<A, T>(&self, args: A) -> Result<T, Error>
where
    A: ArgList,
    T: TryConvert,
{
    ...
    .and_then(|v| v.try_convert())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. and added it for all the other methods.

One thing I also added in relation to this is impl TryConvert for (), because of ergonomics.

Right now this checks for is_nil, but not sure if thats necessary or not, you call.

@ms-jpq ms-jpq force-pushed the main branch 5 times, most recently from 0eebfc4 to 7bb6841 Compare July 18, 2022 16:19
@ms-jpq
Copy link
Contributor Author

ms-jpq commented Jul 18, 2022

@ms-jpq Thanks for your work here!

It would be helpful to split this PR into multiple PRs — one for IO, Fiber, etc. Would make it easier to review.

yeah, I will try.

Im actually using this repo for work project right now, so its a bit hard for me to do that and keep everything in sync in a timely manner since Im reassigned temporarily.

Edit, ok yeah

I just added a whole bunch more surface area. This PR needs to be split up.

@ms-jpq
Copy link
Contributor Author

ms-jpq commented Jul 19, 2022

Seems like some APIs that should be available on windows were missing, and failing the CI.

oxidize-rb/rb-sys#66

Edit resolved.

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.

3 participants