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

make @defnull's proposition into a PR #476

Closed

Conversation

einhirn
Copy link
Contributor

@einhirn einhirn commented Mar 4, 2021

I applied @defnull's code proposition from #108 to current master. Maybe this time the PR will be accepted.

@einhirn
Copy link
Contributor Author

einhirn commented Mar 4, 2021

Sorry - I don't understand rubocop's error message. Can someone with ruby knowledge help me fix this?

Nevermind I've got it now. Missed a line when copy&pasting - that's why lint couldn't make heads&tails of the code.

@einhirn einhirn force-pushed the better-load-computation branch 2 times, most recently from b92ce24 to 9598130 Compare March 4, 2021 11:30
@defnull
Copy link
Contributor

defnull commented Mar 4, 2021

We had this in production, but removed it again because it did not behave well in the situation described by follow-up comments. Also, my code sucks, I'm not a ruby developer.

@einhirn
Copy link
Contributor Author

einhirn commented Mar 4, 2021

We had this in production, but removed it again because it did not behave well in the situation described by follow-up comments. Also, my code sucks, I'm not a ruby developer.

Combined with the newly added "assume configurable base load for the first few minutes"-feature it could work a bit better.

BTW: I don't know how to better attribute you, other than mentioning...

@einhirn
Copy link
Contributor Author

einhirn commented Mar 4, 2021

I guess this should also use

config.x.variable_name

instead of fetching Environment with every poll?

[x] done.

@defnull
Copy link
Contributor

defnull commented Mar 4, 2021

BTW: I don't know how to better attribute you, other than mentioning...

No worries, I don't care much about attribution here. I also signed a CLA so this PR being based on my work should not be a problem :)

@einhirn einhirn force-pushed the better-load-computation branch 3 times, most recently from 02c9b1e to 3ea8be6 Compare March 4, 2021 12:39
@defnull
Copy link
Contributor

defnull commented Mar 4, 2021

This looks wrong: load += weight_users * total_attendees. The total attendees number grows with each meeting, so you're counting users multiple times.

@einhirn
Copy link
Contributor Author

einhirn commented Mar 4, 2021

This looks wrong: load += weight_users * total_attendees. The total attendees number grows with each meeting, so you're counting users multiple times.

Thanks for pointing that out - I'll adress that issue.

[x] done. (having a hard time getting the indentation right.)

@einhirn einhirn force-pushed the better-load-computation branch 3 times, most recently from 16a174f to 069a4c9 Compare March 4, 2021 21:15
@einhirn einhirn force-pushed the better-load-computation branch from 069a4c9 to d138b97 Compare June 8, 2021 07:53
@einhirn einhirn force-pushed the better-load-computation branch 2 times, most recently from 39c6238 to 330d7b8 Compare July 7, 2021 05:49
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@einhirn einhirn force-pushed the better-load-computation branch from 330d7b8 to 29acc01 Compare September 14, 2021 09:33
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jfederico jfederico closed this Sep 30, 2021
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