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

Simplify socket path code #5120

Merged
merged 5 commits into from
May 5, 2023
Merged

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Apr 18, 2023

Instead of reading CARDANO_NODE_SOCKET_PATH in every command, do it once in main and then use it as a default value in --socket-path option if available.

Also read CARDANO_NODE_NETWORK_ID and use that if neither --testnet-magic nor --mainnet supplied.

@newhoggy newhoggy marked this pull request as ready for review April 18, 2023 11:05
@newhoggy newhoggy force-pushed the newhoggy/simplify-socket-path-code branch from fb637c6 to 1388ee2 Compare April 18, 2023 11:11
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

I'll go out on a limb and claim that it is never necessary to use unsafePerformIO in application code. And most uses do bite you in the end (often when refactoring later).

We should just do it properly.

@newhoggy
Copy link
Contributor Author

newhoggy commented Apr 18, 2023

I understand that most uses case bite you in the end, but the one use case we're you using it to initialise a constant that never changes isn't one of them and I never use it for any other reason.

@dcoutts
Copy link
Contributor

dcoutts commented Apr 18, 2023

Our attitude should be to simply not use it (in application code), rather than looking for justification as to why it might not be so bad in this or that case.

It's not "Haskell". It has no semantics.

@newhoggy newhoggy force-pushed the newhoggy/simplify-socket-path-code branch 6 times, most recently from e32722d to 9d8517a Compare April 19, 2023 04:59
@newhoggy newhoggy requested review from carbolymer and dcoutts April 19, 2023 04:59
@newhoggy
Copy link
Contributor Author

The code no longer uses unsafePerformIO.

@newhoggy newhoggy force-pushed the newhoggy/simplify-socket-path-code branch 2 times, most recently from 9c9d7c5 to 9e44345 Compare April 19, 2023 12:23
@newhoggy newhoggy force-pushed the newhoggy/simplify-socket-path-code branch 2 times, most recently from 03785be to c71ab32 Compare April 27, 2023 01:13
@newhoggy newhoggy force-pushed the newhoggy/simplify-socket-path-code branch from c71ab32 to a11f750 Compare May 3, 2023 22:38
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice 👍. Just one comment for you to consider.

, Opt.help $ mconcat
[ "Path to the node socket. This overrides the CARDANO_NODE_SOCKET_PATH "
, "environment variable"
pSocketPath :: EnvCli -> Parser SocketPath
Copy link
Contributor

@Jimbo4350 Jimbo4350 May 4, 2023

Choose a reason for hiding this comment

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

Why not leverage Alternative (<|>)?:

pSocketPath :: EnvCli -> Parser SocketPath
pSocketPath envCli =
  let opts = mconcat
              [ Opt.long "socket-path"
              , Opt.metavar "SOCKET_PATH"
              , Opt.help $ mconcat
                [ "Path to the node socket.  This overrides the CARDANO_NODE_SOCKET_PATH "
                , "environment variable"
                ]
              , Opt.completer (Opt.bashCompleter "file")
              ]
  in fmap SocketPath (Opt.strOption opts) <|>
     -- Default to the socket path specified by the environment variable if it is available.
     pure (maybe (error "No socket path specified via cli nor env var") SocketPath $ envCliSocketPath envCli)

And then we can error immediately rather than the networking code throw some obscure error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already use Alternative via asum. If CARDANO_NODE_SOCKET_PATH is not supplied, the code as it currently is will error immediately because --socket-path is the only Alternative in this case. The CLI parsing will fail because --socket-path is not supplied and it is the only alternative. You only get the fallback alternative if CARDANO_NODE_SOCKET_PATH is supplied, in which case the socket path provided by the environment variable is the default and --socket-path will override.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the CLI help message to be clearer.

@newhoggy newhoggy added this pull request to the merge queue May 5, 2023
@newhoggy newhoggy removed this pull request from the merge queue due to a manual request May 5, 2023
@newhoggy newhoggy force-pushed the newhoggy/simplify-socket-path-code branch from a11f750 to 9a195fb Compare May 5, 2023 06:20
@newhoggy newhoggy enabled auto-merge May 5, 2023 06:21
@newhoggy newhoggy added this pull request to the merge queue May 5, 2023
Merged via the queue into master with commit 510ecbf May 5, 2023
@iohk-bors iohk-bors bot deleted the newhoggy/simplify-socket-path-code branch May 5, 2023 08:00
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.

4 participants