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

refactor: Upgrade kclvm/cmd clap version to 4.x #560

Merged
merged 1 commit into from May 31, 2023
Merged

refactor: Upgrade kclvm/cmd clap version to 4.x #560

merged 1 commit into from May 31, 2023

Conversation

niconical
Copy link
Contributor

@niconical niconical commented May 28, 2023

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

fix #333

2. What is the scope of this PR:

  • kclvm/Cargo.lock
  • kclvm/cmd/Cargo.toml
  • kclvm/cmd/src/fmt.rs
  • kclvm/cmd/src/lib.rs
  • kclvm/cmd/src/lint.rs
  • kclvm/cmd/src/settings.rs
  • kclvm/cmd/src/util.rs
  • kclvm/cmd/src/vet.rs

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

Summary of changes:

  • Use Arg.num_args
  • Use ArgMatches.get_one and ArgMatches.get_many in place of ArgMatches.value_of* and ArgMatches.values_of*
  • Replace occurrences_of with ArgAction::Count and get_count for flag counting
  • Replace occurrences_ofwith get_flag for flags whether

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

This PR updates the dependency of clap from 3.x to 4.x in kclvm/cmd, resulting in significant changes to the CLI interface style, such as color support.

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

If there are any defects in this PR, please contact me in time.

6. Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@niconical
Copy link
Contributor Author

/assign @Peefy

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 5103381836

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.988%

Totals Coverage Status
Change from base Build 5065456266: 0.0%
Covered Lines: 2300
Relevant Lines: 2614

💛 - Coveralls

@Peefy
Copy link
Contributor

Peefy commented May 28, 2023

/assign @Peefy

Thank you for your contribution. I have allowed testing and you can execute make fmt to format the submitted code.

@niconical
Copy link
Contributor Author

niconical commented May 28, 2023

I have discovered some issues while executing the example,I will solve them.

@Peefy
Copy link
Contributor

Peefy commented May 28, 2023

I have discovered some issues while executing the example,I will solve them.

OK, go ahead!

@niconical niconical changed the title [PTAL] feat: Upgrade kclvm/cmd clap version to 4.3.0 [WIP] feat: Upgrade kclvm/cmd clap version to 4.3.0 May 28, 2023
@niconical niconical changed the title [WIP] feat: Upgrade kclvm/cmd clap version to 4.3.0 [WIP] feat: Upgrade kclvm/cmd clap version to 4.x May 28, 2023
@niconical niconical changed the title [WIP] feat: Upgrade kclvm/cmd clap version to 4.x [WIP] refactor: Upgrade kclvm/cmd clap version to 4.x May 28, 2023
@Peefy Peefy self-requested a review May 29, 2023 02:48
@Peefy
Copy link
Contributor

Peefy commented May 30, 2023

@niconical CI failed, corresponding test cases need to be modified.

@kcl-lang kcl-lang deleted a comment from niconical May 31, 2023
@niconical niconical changed the title [WIP] refactor: Upgrade kclvm/cmd clap version to 4.x refactor: Upgrade kclvm/cmd clap version to 4.x May 31, 2023
@niconical niconical changed the title refactor: Upgrade kclvm/cmd clap version to 4.x [WIP] refactor: Upgrade kclvm/cmd clap version to 4.x May 31, 2023
@niconical niconical changed the title [WIP] refactor: Upgrade kclvm/cmd clap version to 4.x refactor: Upgrade kclvm/cmd clap version to 4.x May 31, 2023
Signed-off-by: cuih <cuihang97@foxmail.com>
@niconical niconical changed the title refactor: Upgrade kclvm/cmd clap version to 4.x [WIP]refactor: Upgrade kclvm/cmd clap version to 4.x May 31, 2023
@niconical niconical changed the title [WIP]refactor: Upgrade kclvm/cmd clap version to 4.x refactor: Upgrade kclvm/cmd clap version to 4.x May 31, 2023
@niconical
Copy link
Contributor Author

niconical commented May 31, 2023

I noticed a FAILED reported in CI

FAILED test_grammar.py::test_grammar[/home/ch/kcl/test/grammar/builtins/json/output_2] - ruamel.yaml.parser.ParserError: expected '<document start>', but found ('<scalar>',)

But when I use the latest kcl release version to execute cd kcl/kclvm/tests/integration/grammar && kclvm -m pytest -v -n 5, I still encounter the same problem.

Here are some environment parameters:

  1. The version of the kcl release I used
$ kclvm_cli version
Version: 0.5.0-alpha.1-eb2f4d1aabc2a287c298bc35c5fcfec1
Platform: x86_64-unknown-linux-gnu
GitCommit: VERGEN_IDEMPOTENT_OUTPUT
  1. My OS environment
$ cat /etc/os-release
PRETTY_NAME="Ubuntu 22.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.2 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy

@Peefy
Copy link
Contributor

Peefy commented May 31, 2023

I noticed a FAILED reported in CI

FAILED test_grammar.py::test_grammar[/home/ch/kcl/test/grammar/builtins/json/output_2] - ruamel.yaml.parser.ParserError: expected '<document start>', but found ('<scalar>',)

But when I use the latest kcl release version to execute cd kcl/kclvm/tests/integration/grammar && kclvm -m pytest -v -n 5, I still encounter the same problem.

Here are some environment parameters:

  1. The version of the kcl release I used
$ kclvm_cli version
Version: 0.5.0-alpha.1-eb2f4d1aabc2a287c298bc35c5fcfec1
Platform: x86_64-unknown-linux-gnu
GitCommit: VERGEN_IDEMPOTENT_OUTPUT
  1. My OS environment
$ cat /etc/os-release
PRETTY_NAME="Ubuntu 22.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.2 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy

I noticed a FAILED reported in CI

FAILED test_grammar.py::test_grammar[/home/ch/kcl/test/grammar/builtins/json/output_2] - ruamel.yaml.parser.ParserError: expected '<document start>', but found ('<scalar>',)

But when I use the latest kcl release version to execute cd kcl/kclvm/tests/integration/grammar && kclvm -m pytest -v -n 5, I still encounter the same problem.

Here are some environment parameters:

  1. The version of the kcl release I used
$ kclvm_cli version
Version: 0.5.0-alpha.1-eb2f4d1aabc2a287c298bc35c5fcfec1
Platform: x86_64-unknown-linux-gnu
GitCommit: VERGEN_IDEMPOTENT_OUTPUT
  1. My OS environment
$ cat /etc/os-release
PRETTY_NAME="Ubuntu 22.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.2 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy

Thank you for your feedback. After investigation, it has been found that there has been a change in the logic of the upstream YAML library. I have temporarily fixed the version of ruamel.yaml in CI to 0.16.12 to bypass it. I have reviewed your code and I will merge it. ❤️

Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

@Peefy Peefy merged commit 802c52a into kcl-lang:main May 31, 2023
7 of 11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] KCLVM CLI clap deps is too low and need to upgrade the clap version.
3 participants