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

Rename the non-exposed field "count" to "size" #6048

Merged
merged 2 commits into from Jan 7, 2020

Conversation

sylr
Copy link
Contributor

@sylr sylr commented Dec 17, 2019

What does this PR do?

This PR renames a variable which name led to believe it contains a count whereas it contains a size.

Motivation

I've lost quite a bit of time while debuging thinking that crr.count was wrongly incremented.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

Hello,

The namecount is because this variable is used to count the number of byte already read.

At the end of the read, the number of byte read will be the size of the request body.

As it's an incremental thing, the name count is more appropriated than size for the captureRequestReader.

But for the request type the value is set only one time so the name size can be more appropriated.

So could to rename size to count for the captureRequestReader.

@ldez ldez added this to To review in v2 via automation Jan 7, 2020
@ldez ldez added this to the next milestone Jan 7, 2020
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez added the kind/enhancement a new or improved feature. label Jan 7, 2020
@ldez ldez changed the title Change variable name Rename rename the non-exposed field "count" to "size" Jan 7, 2020
@ldez ldez changed the title Rename rename the non-exposed field "count" to "size" Rename the non-exposed field "count" to "size" Jan 7, 2020
Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Sylvain Rabot <s.rabot@lectra.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v2
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants