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

Linter and formatting fixes #2995

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Linter and formatting fixes #2995

merged 2 commits into from
Mar 31, 2023

Conversation

na--
Copy link
Member

@na-- na-- commented Mar 31, 2023

I got annoyed by some of the many linter errors we have unresolved. Some of them like the loopclosure ones might even hide other problems, so I decided it's time to fix them.

I'll try to do fixes for various unrelated issues commit by commit. To reduce merge conflicts, this is built on top of #2960.

@na-- na-- added this to the v0.44.0 milestone Mar 31, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #2995 (384a8d5) into master (c09919b) will increase coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 384a8d5 differs from pull request most recent head 07630ed. Consider uploading reports for the commit 07630ed to get more accurate results

@@            Coverage Diff             @@
##           master    #2995      +/-   ##
==========================================
+ Coverage   76.90%   76.99%   +0.08%     
==========================================
  Files         228      228              
  Lines       17050    17050              
==========================================
+ Hits        13113    13128      +15     
+ Misses       3090     3079      -11     
+ Partials      847      843       -4     
Flag Coverage Δ
ubuntu 76.99% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/common/bridge.go 100.00% <ø> (ø)
js/eventloop/eventloop.go 100.00% <ø> (ø)
js/summary.go 89.87% <ø> (ø)
lib/execution_segment.go 92.73% <ø> (ø)
lib/executor/constant_arrival_rate.go 96.68% <ø> (ø)
lib/executor/externally_controlled.go 77.37% <ø> (ø)
lib/executor/helpers.go 100.00% <ø> (ø)
lib/executor/per_vu_iterations.go 97.47% <ø> (ø)
lib/executor/ramping_arrival_rate.go 96.55% <ø> (ø)
lib/executor/ramping_vus.go 95.36% <ø> (ø)
... and 10 more

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@na-- na-- force-pushed the logrus-refactor branch 2 times, most recently from 3c2733c to f577879 Compare March 31, 2023 10:14
Base automatically changed from logrus-refactor to master March 31, 2023 10:36
imiric
imiric previously approved these changes Mar 31, 2023
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

This includes changes from the other PRs, so I ignored those, but the two top-most commits LGTM.

imiric
imiric previously approved these changes Mar 31, 2023
@na-- na-- requested a review from codebien March 31, 2023 11:28
mstoykov
mstoykov previously approved these changes Mar 31, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I don't really like that we are breaking the graphs in ramping_vus.go. But if you want you can:

  1. skip that file for now
  2. fix them in a separate PR - but open an issue for that.

I am approving this in case you go with 2

lib/executor/ramping_vus.go Outdated Show resolved Hide resolved
// 3| ***************
// 2| *****************
// 1| *******************
// 0------------------------> time(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 0------------------------> time(s)
// 0+-----------------------> time(s)

Wouldn't that be better 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

not really, it might confuse someone

lib/executor/ramping_vus.go Outdated Show resolved Hide resolved
The only exceptions were some `go generate`d code.
@na-- na-- dismissed stale reviews from mstoykov and imiric via 07630ed March 31, 2023 11:41
@na-- na-- requested review from mstoykov and imiric March 31, 2023 11:41
@na-- na-- merged commit 988f2f6 into master Mar 31, 2023
@na-- na-- deleted the linter-mega-fixes branch March 31, 2023 12:31
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.

4 participants