ubiquity / ubiquibot

Putting the 'A' in 'DAO'
https://github.com/marketplace/ubiquibot
MIT License
17 stars 60 forks source link

Config Name Format #761

Closed Sadaf-A closed 11 months ago

Sadaf-A commented 1 year ago

kebab case to camel case

Resolves #711

netlify[bot] commented 1 year ago

Deploy Preview for ubiquibot-staging ready!

Name Link
Latest commit c9b8780176ca76e2b7998877ba4aae3387f01b17
Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/650d064fe7f3330008d7818d
Deploy Preview https://deploy-preview-761--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.

Sadaf-A commented 1 year ago

1/ You should not change the name of ubiquibot-config.yml 2/ I can see no changes in config names. 3/ Please make sure you mark the PR ready for review once done.

i changed price-multiplier to priceMultiplier and could you please elaborate a little on where are the config names that I am supposed to change

EtherealGlow commented 12 months ago

Shouldn't you change every config name

Sadaf-A commented 12 months ago

Shouldn't you change every config name

I'm sorry but not able to understand can you link the file?

0xcodercrane commented 12 months ago

We should change every config name in ubiquibot-config.yml and codebase to read them in src/utils/private.ts

EtherealGlow commented 12 months ago

Maybe also in default config json file

0xcodercrane commented 12 months ago

Maybe also in default config json file

True

0xcodercrane commented 12 months ago

bump @EtherealGlow cause its blocking other PR from the merge.

0xcodercrane commented 12 months ago

In the recent merge, we've changed the default config filename from *.json to *.ts. would you get the changes into your branch? @Sadaf-A

Sadaf-A commented 12 months ago

In the recent merge, we've changed the default config filename from *.json to *.ts. would you get the changes into your branch? @Sadaf-A

sure, I'll get to it right now

0xcodercrane commented 11 months ago

same feeling yeah. so my idea is to create a sync PR from development to main as soon as this PR merged.

0xcodercrane commented 11 months ago

I am gonna handle the merge and hotfix if needed tomorrow morning.

0xcodercrane commented 11 months ago

@rndquu would you open the config PRs in both ubiquity and ubiquibot org for the upcoming changes?

rndquu commented 11 months ago

@rndquu would you open the config PRs in both ubiquity and ubiquibot org for the upcoming changes?

I remember pavlovcik explicitly stated that the config should be kebab-case hence this refactoring was implemented. You 100% sure that we should migrate to camel case?

From my point of view it looks like that every 6 months we refactor the bot's config from camel case to kebab case and back which doesn't bring any advantages.

0xcodercrane commented 11 months ago

Its kind of back and forth but I hope this would be the final one in case changes.

rndquu commented 11 months ago

Its kind of back and forth but I hope this would be the final one in case changes.

We need @pavlovcik's review for this PR

0x4007 commented 11 months ago

@rndquu would you open the config PRs in both ubiquity and ubiquibot org for the upcoming changes?

I remember pavlovcik explicitly stated that the config should be kebab-case hence this refactoring was implemented. You 100% sure that we should migrate to camel case?

From my point of view it looks like that every 6 months we refactor the bot's config from camel case to kebab case and back which doesn't bring any advantages.

I vaguely recall a discussion we had where it was decided to finally change everything into camelCase recently because the only software that consumes this configuration file is the UbiquiBot typescript code, which expresses the same properties in camelCase.

Sadaf-A commented 11 months ago

CI doesn’t pass but the rest seem fine.

I'll fix that right away!

0xcodercrane commented 11 months ago

CI doesn’t pass but the rest seem fine.

ok will handle the merge after the conflicts are resolved. This should be a top priority task because it blocks some PRs right now.

The next steps:

0xcodercrane commented 11 months ago

https://github.com/ubiquibot/ubiquibot-config/pull/8 https://github.com/ubiquity/ubiquibot-config/pull/15

requested a review from @rndquu

0xcodercrane commented 11 months ago

merging the both config PRs.

0xcodercrane commented 11 months ago

https://github.com/ubiquity/ubiquibot/pull/795

sync PR

0xcodercrane commented 11 months ago

795

sync PR

merged

0xcodercrane commented 11 months ago

I had quick QAs once I merged the relevant PRs. Here are the QA issues.

Staging: https://github.com/ubiquibot/staging/issues/179 Production: https://github.com/ubiquibot/production/issues/65

So far looks good to me.

EtherealGlow commented 11 months ago

Finally