Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

fix: derive weight automatically #627

Merged

Conversation

kamaalsultan
Copy link
Contributor

Resolves #495

@netlify
Copy link

netlify bot commented Aug 17, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit 3e40c9c
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/64e864c9f33f740008aa52e8
😎 Deploy Preview https://deploy-preview-627--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Priority weight was not implemented according to the spec:

During runtime, when a bounty is being assistive priced by the bot, the bot should parse the number from the Priority label and use it in place of the weight

Did you accommodate the following?

Any words should be ignored from this parser, except for Priority: of course, in order to understand that it is a priority label.


src/helpers/shared.ts Outdated Show resolved Hide resolved
src/helpers/shared.ts Outdated Show resolved Hide resolved
@kamaalsultan
Copy link
Contributor Author

kamaalsultan commented Aug 17, 2023

Bounty price is depend on both weight from priority and time label.
const price = 1000 * base * timeValue * priority;
Here, timeValue is weight from time label. If we want to remove weight from time label, pricing calculation should be changed too.
What do you think, @pavlovcik ?

src/helpers/shared.ts Outdated Show resolved Hide resolved
@whilefoo
Copy link
Collaborator

https://github.com/ubiquity/ubiquibot/blob/f6e99dfb406559a044969bb82aff478daae172cd/src/handlers/shared/pricing.ts#L11C1-L11C1

@pavlovcik it appears we only use weight from priority and time labels to calculate the price. We only use the value to calculate end date.

@0x4007
Copy link
Member

0x4007 commented Aug 21, 2023

https://github.com/ubiquity/ubiquibot/blob/f6e99dfb406559a044969bb82aff478daae172cd/src/handlers/shared/pricing.ts#L11C1-L11C1

@pavlovcik it appears we only use weight from priority and time labels to calculate the price. We only use the value to calculate end date.

This looks like a mistake. One function is to set the price, the other function is to get the price. Both should be using value or weight.

@kamaalsultan
Copy link
Contributor Author

kamaalsultan commented Aug 21, 2023

https://github.com/ubiquity/ubiquibot/blob/f6e99dfb406559a044969bb82aff478daae172cd/src/handlers/shared/pricing.ts#L11C1-L11C1
@pavlovcik it appears we only use weight from priority and time labels to calculate the price. We only use the value to calculate end date.

This looks like a mistake. One function is to set the price, the other function is to get the price. Both should be using value or weight.

calculateBountyPrice uses weight to calculate price. No function is used to set weight...

@whilefoo
Copy link
Collaborator

Both are using weight. The timeValue parameter in calculateBountyPrice is actually passed weight in

const previousTargetPrice = calculateBountyPrice(timeLabel.weight, priorityLabel.weight, previousBaseRate);

@0x4007
Copy link
Member

0x4007 commented Aug 22, 2023

Both are using weight

We should rename it to value and then fully get rid of weight

@kamaalsultan
Copy link
Contributor Author

kamaalsultan commented Aug 22, 2023

We should rename it to value and then fully get rid of weight

Should we rename weight to value in priority and remove weight from time?

@kamaalsultan
Copy link
Contributor Author

kamaalsultan commented Aug 22, 2023

We should rename it to value and then fully get rid of weight

Please note that weight of both labels are used to calculate price and value in time label is used to calculate deadline. We can remove both weight and value and make a simple helper function that calculates them from label name. @pavlovcik

@0x4007
Copy link
Member

0x4007 commented Aug 22, 2023

We should rename it to value and then fully get rid of weight

Please note that weight of both labels are used to calculate price and value in time label is used to calculate deadline. We can remove both weight and value and make a simple helper function that calculates them from label name. @pavlovcik

Yes that is what the specification says. The only adjustment I have is to rename the property name from weight to value

@kamaalsultan
Copy link
Contributor Author

I removed both value and weight and made simple function that calculates them.

@0x4007 0x4007 requested a review from web4er August 24, 2023 00:42
@0x4007
Copy link
Member

0x4007 commented Aug 24, 2023

@whilefoo you think you can re-review?

@kamaalsultan
Copy link
Contributor Author

@whilefoo you think you can re-review?

Please note that I made a simple function that calculates value from time label name not using ms library to make clearer. It will conflict with #638 . One of #627 and #638 might need modifications after another one is merged.

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

This implementation looks pretty clean. Just needs QA.

src/helpers/shared.ts Show resolved Hide resolved
src/helpers/shared.ts Show resolved Hide resolved
@Draeieg
Copy link
Contributor

Draeieg commented Aug 24, 2023

ubiquibot/staging#144 tags haven't been changed in accordance to the new weights which causes the (old) 0 normal tag to not price the issue @ByteBallet

@kamaalsultan
Copy link
Contributor Author

kamaalsultan commented Aug 24, 2023

ubiquibot/staging#144 tags haven't been changed in accordance to the new weights which causes the (old) 0 normal tag to not price the issue @ByteBallet

I thought, we are going to shift the priority tag.
#627 (comment)

So in this case we would shift them all up by one, meaning, Priority: 1 (Normal) would be the first value, and we can delete the weight property.

#383 (comment)

@Draeieg

@Draeieg
Copy link
Contributor

Draeieg commented Aug 24, 2023

ubiquibot/staging#144 tags haven't been changed in accordance to the new weights which causes the (old) 0 normal tag to not price the issue @ByteBallet

I thought, we are going to shift the priority tag. #627 (comment)

So in this case we would shift them all up by one, meaning, Priority: 1 (Normal) would be the first value, and we can delete the weight property.

#383 (comment)

@Draeieg

@pavlovcik if this is OK then QA is approved

@0x4007
Copy link
Member

0x4007 commented Aug 25, 2023

ubiquibot/staging#144 tags haven't been changed in accordance to the new weights which causes the (old) 0 normal tag to not price the issue @ByteBallet

Yeah we can update the priority numbers.

@whilefoo if you approve please merge.

@whilefoo whilefoo merged commit fb1599b into ubiquity:development Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Refactor Config: Automatically Derive weight / ref
4 participants