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

Utility: use Fsdk #55

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

webwarrior-ws
Copy link

@webwarrior-ws webwarrior-ws commented Jan 9, 2023

Added fsdk package. Reuse functions from Fsdk.

Renamed FSharpUtil module to AsyncUtil.

Changed Retry function to take 1 type parameter instead of 2 as handling 2 types of exceptions is not needed anymore because SocketException is wrapped in NOnionException.

@aarani
Copy link
Collaborator

aarani commented Jan 9, 2023

LGTM

NOnion/Utility/FSharpUtil.fs Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Jan 9, 2023

@webwarrior-ws can you also fix the conventions job with a similar commit to this please: 37e374a7b03cb55b4718c46e65911f09c21f876b (and move it to be the first commit).

@webwarrior-ws webwarrior-ws force-pushed the fsharputil-retry branch 2 times, most recently from 05bd5a1 to c99a359 Compare January 9, 2023 13:34
@webwarrior-ws
Copy link
Author

Should be good now

@knocte
Copy link
Member

knocte commented Jan 9, 2023

LGTM

Please approve via GitHub PR approval process.

@knocte
Copy link
Member

knocte commented Jan 11, 2023

Please approve via GitHub PR approval process.

@aarani ^ otherwise I get "Merging is blocked" in the GitHub UI.

@webwarrior-ws
Copy link
Author

Can't use Fsdk because it requires FSharp.Core 6.0, but NOnion uses 4.7.2 (in CI at least).

@knocte
Copy link
Member

knocte commented Jan 16, 2023

Upgrade FSharp.Core then?

@webwarrior-ws webwarrior-ws force-pushed the fsharputil-retry branch 3 times, most recently from f9d5866 to 35d2ec5 Compare January 16, 2023 13:27
@knocte
Copy link
Member

knocte commented Jan 18, 2023

Thinking about it more, please revert your last commit that removes NOnion.Utility.FSharpUtil.WithTimeout, but I want FSharpUtil module be gone from NOnion to prevent naming clash with Fsdk, so let's rename it to NetworkUtils.fs? (To be consistent with other files in that folder.) BTW I had a look at other files in that folder and I think ResultUtils.fs's OperationResult overlaps with FSharpUtil's Either, let's remove it and use Either. And same thing for the Unwrap function, I think FSharpUtil has Unwrap func? If not, let's move it to Fsdk.

Changed Retry function to take 1 type parameter instead of 2 as
handling 2 types of exceptions is not needed anymore because
SocketException is wrapped in NOnionException.
Added fsdk package. Reuse functions from Fsdk.
Renamed FSharpUtil module to AsyncUtil.
@webwarrior-ws webwarrior-ws changed the title Utility: made Retry func take 1 type parameter Utility: use Fsdk May 29, 2023
Use ExtractEmbeddedResourceFileContents function from Fsdk.
Remove local copy of that function.
@aarani aarani mentioned this pull request May 31, 2023
@knocte
Copy link
Member

knocte commented Oct 21, 2023

Thinking about it more, please revert your last commit that removes NOnion.Utility.FSharpUtil.WithTimeout, but I want FSharpUtil module be gone from NOnion to prevent naming clash with Fsdk, so let's rename it to NetworkUtils.fs? (To be consistent with other files in that folder.) BTW I had a look at other files in that folder and I think ResultUtils.fs's OperationResult overlaps with FSharpUtil's Either, let's remove it and use Either. And same thing for the Unwrap function, I think FSharpUtil has Unwrap func? If not, let's move it to Fsdk.

Was this done?

@webwarrior-ws
Copy link
Author

Thinking about it more, please revert your last commit that removes NOnion.Utility.FSharpUtil.WithTimeout, but I want FSharpUtil module be gone from NOnion to prevent naming clash with Fsdk, so let's rename it to NetworkUtils.fs? (To be consistent with other files in that folder.) BTW I had a look at other files in that folder and I think ResultUtils.fs's OperationResult overlaps with FSharpUtil's Either, let's remove it and use Either. And same thing for the Unwrap function, I think FSharpUtil has Unwrap func? If not, let's move it to Fsdk.

Was this done?

First part - yes, FSharpUtil is now called AsyncUtil.
OperationsResult is still there; it has different signature (1 type parameter instead of 2 in Fsdk.Either).

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