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

c-s: parser refactor #47

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

muzarski
Copy link
Collaborator

Motivation

Before the changes, the parser would only validate the input, returning the validated string to the user (which still needs to be parsed to the according type).

Changes

This PR introduces the changes that address this issue. The major change is the introduction of Parsable trait described in the code. The SimpleParam is now generic over Parsable trait.

Most of the changes include adjusting the rest of the code to Parsable trait + some fixups.

  • Introduced Parsable trait
  • Made SimpleParam be generic over Parsable
  • Adjusted the rest of the code to usage of generic SimpleParam
  • some fixups, such as changing the types of duration, or consistency_level parameters

Changed error print to `:?` format, which displays the context stack.
Adjusted the types of `duration`, `consistency_level` and `serial_consistency_level` parameters.
- `duration`: from `u64` to `std::time::Duration`.
- `consistency_level`: from `ConsistencyLevel` to `scylla::statement::Consistency`.
- `serial_consistency_level`: from `SerialConsistencyLevel` to `scylla::statement::SerialConsistency`.
Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

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

Just one nitpick.

@muzarski muzarski force-pushed the cassandra-stress-cli-parser-refactor branch from b082ae4 to 8479f91 Compare August 28, 2023 13:04
This commit introduces `Parsable` trait. It's implemented for the types
that handle the string-to-Parsable::Parsed parsing logic.

In this commit, we implement it for the most common types, as well as
for some enum types such as `ConsistencyLevel`, etc.
Before this commit, the parser would only be responsible for input VALIDATION.
The parameters' handles would return the strings that were matched to the
given parameter (and validated). However, the ideal approach is so the
user can retrieve the parsed value instead of validated String. This commit
introduces the changes addressing this issue.

The `SimpleParam` is now generic over `Parsable` trait, which is responsible
for parsing strings to the specified type (`type Parsable::Parsed`).

Adjusted the rest of the code to this change, including fixups in
read/write command parsing.
@muzarski muzarski force-pushed the cassandra-stress-cli-parser-refactor branch from 8479f91 to cd409f9 Compare August 29, 2023 09:52
@muzarski
Copy link
Collaborator Author

v2:

  • Fixed review comments

@muzarski muzarski requested a review from piodul August 29, 2023 11:17
@piodul piodul merged commit 3ef6844 into scylladb:master Aug 30, 2023
1 check passed
@muzarski muzarski deleted the cassandra-stress-cli-parser-refactor branch September 13, 2023 12:06
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