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

Add deprecation info of version flag #2834

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

Conversation

kkawula
Copy link
Collaborator

@kkawula kkawula commented Jan 9, 2025

Closes #2828

Introduced changes

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@kkawula kkawula changed the title version deprecation info Add deprecation info of version flag Jan 9, 2025
@kkawula kkawula marked this pull request as ready for review January 9, 2025 11:24
crates/sncast/tests/e2e/account/deploy.rs Outdated Show resolved Hide resolved
crates/sncast/src/helpers/version.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ksew1 ksew1 Jan 9, 2025

Choose a reason for hiding this comment

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

It think this would be more readable instead of current macro

use anyhow::Error;
use clap::ValueEnum;
use shared::print::print_as_warning;

const DEPRECATION_MESSAGE: &str = "The '--version' flag is deprecated and will be removed in the future. Version 3 will become the only type of transaction available.";

pub fn parse_version<T: ValueEnum>(s: &str) -> Result<T, String> {
    print_as_warning(&Error::msg(DEPRECATION_MESSAGE));

    let ignore_case = false; // please check if it should be true of false to not make breaking changes

    T::from_str(s, ignore_case)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, I think it would be better to change the signature of print_as_warning from:

pub fn print_as_warning(error: &Error)

to:

pub fn print_as_warning(error: impl Display)

This way, we wouldn't need to use the awkward workaround &Error::msg(DEPRECATION_MESSAGE) and could simply pass DEPRECATION_MESSAGE directly 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, it will simplify a lot, also some tests can be removed then

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

  1. Shouldn't we update this message then? If someone uses --token we say Use '--version' instead while version will be deprecated too. I think we can just remove Use '--version' instead sentence.
  2. CI is failing 👀

Otherwise looks fine 👍

@kkawula kkawula marked this pull request as draft January 10, 2025 08:07
@kkawula kkawula marked this pull request as ready for review January 10, 2025 10:01
Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts in changelog

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.

Deprecate versions other than v3 in sncast commands that accept --version
4 participants