-
Notifications
You must be signed in to change notification settings - Fork 13
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
Deprecate --protocol-params-file
option of the transaction build
command more thoroughly
#28
Deprecate --protocol-params-file
option of the transaction build
command more thoroughly
#28
Conversation
--protocol-params-file
option of the transaction build
command more thoroughly
…and more thoroughly
c1b9737
to
f9ebbe5
Compare
@@ -232,7 +234,7 @@ data TransactionCmd | |||
[ScriptFile] | |||
-- ^ Auxiliary scripts | |||
[MetadataFile] | |||
(Maybe ProtocolParamsFile) | |||
(Maybe (Deprecated ProtocolParamsFile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it more obvious a field is deprecated.
, "retrieved from the node." | ||
] | ||
, Opt.completer (Opt.bashCompleter "file") | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated options should have help that reflects that.
mUpperBound certs wdrls metadataSchema scriptFiles metadataFiles mDeprecatedProtocolParamsFile | ||
mUpProp outputOptions = do | ||
forM_ mDeprecatedProtocolParamsFile $ \_ -> | ||
liftIO $ printWarning "'--protocol-params-file' for 'transaction build' is deprecated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warn early rather than print warnings deep within the call hierarchy and after doing work.
mUpdatePropF mOverrideWits outputOptions = do | ||
|
||
liftIO $ forM_ mpparams $ \_ -> | ||
printWarning "'--protocol-params-file' for 'transaction build' is deprecated" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mpparams
is never used except for this warning, so don't pass it in the first place.
Filepath of the JSON-encoded protocol parameters file | ||
Filepath of the JSON-encoded protocol parameters | ||
file. DEPRECATED The option is ignored and protocol | ||
parameters are retrieved from the node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. What we should start doing is also specifying in the warning which release we intend to move from deprecation to removal. I think in the past we agreed on 2 releases?
@Jimbo4350 should we leave "better deprecation" message or remove the argument altogether? #34 |
Description
Changelog
Checklist
See Runnings tests for more details
CHANGELOG.md
for affected package.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7
Note on CI
If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.