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 speed setter and getter #2

Merged
merged 4 commits into from
Jan 19, 2024
Merged

Add speed setter and getter #2

merged 4 commits into from
Jan 19, 2024

Conversation

aesteve-rh
Copy link
Owner

Add setter and getter for gear and speed (and display_speed).

Signed-off-by: Albert Esteve <aesteve@redhat.com>
@aesteve-rh aesteve-rh self-assigned this Jan 18, 2024
Copy link

@barpavel barpavel left a comment

Choose a reason for hiding this comment

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

LGTM.
Some comments might be due to lack of familiarity with the code :)

README.md Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
@aesteve-rh aesteve-rh merged commit 80d3f65 into main Jan 19, 2024
2 checks passed
@aesteve-rh aesteve-rh deleted the add-speed-setter branch January 19, 2024 10:55
@aesteve-rh
Copy link
Owner Author

Thanks @barpavel !

}
}
}

impl GetPropertyValue for EmulatorMessage {
fn get_value(&self) -> Result<VehiclePropertyValue> {
if self.value.len() != 1 {

Choose a reason for hiding this comment

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

If it can't be zero, maybe better change to >1.

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, the only valid value is 1. With >1 it would allow empty vectors.

Copy link

@barpavel barpavel Jan 19, 2024

Choose a reason for hiding this comment

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

No, you misunderstood me.
I meant the following:
!=1 means either 0 or more than 1 (2, 3, ...).
If zero values is possible, then returning ReceiveMessageTooManyValuesError is confusing.
If zero of values is not possible, then changing !=1 into >1 is the same, but more readable.

Choose a reason for hiding this comment

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

But never mind, this is not that important, just explaining myself :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah! gotcha. Well, at least the size of the values vector is printed with the error :D

@barpavel
Copy link

Ahhh, already merged, all good :)
This minor comment is not important or can be addressed later!

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.

None yet

2 participants