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

refactor!: have knuu as an object #356

Merged
merged 12 commits into from
May 29, 2024
Merged

Conversation

mojtaba-esk
Copy link
Contributor

This PR proposes a structural change to the knuu pkg to have it as an object.
This helps to have more idiomatic code and enable us to add unit tests easier. Moreover, since every test can have their own knuu object and handle everything in isolation.

Note 1: in order to keep it backward compatible with the current tests, some wrapper functions with a global var is added, and they are in a number of files with _old.go postfix so we can delete the files when the users are ready to use this one.

Note 2: There was an attempt not to change everything, i.e. the refactor tries to focus on introducing an object structure for knuu. So, we need a number of refactor rounds on the knuu pkg and in general on the repo.

Closes #328

@mojtaba-esk mojtaba-esk requested a review from a team May 21, 2024 16:20
@mojtaba-esk mojtaba-esk self-assigned this May 21, 2024
@celestia-bot celestia-bot requested review from tty47, MSevey and sysrex May 21, 2024 16:20
@MSevey
Copy link
Member

MSevey commented May 22, 2024

Since this is a relatively nuanced why of keeping backwards compatibility I would recommend you add some comments to the top of the _old files and their corresponding files that captures how the files are setup and should be used while we are in this period of maintaining backwards compatibility.

Otherwise I think it puts a lot of pressure and burden on the team to keep the context of this refactor in their minds while working on fixes and features.

@mojtaba-esk mojtaba-esk requested review from smuu and MSevey May 23, 2024 08:47
@mojtaba-esk mojtaba-esk enabled auto-merge May 23, 2024 08:47
@smuu
Copy link
Member

smuu commented May 23, 2024

Is it possible to add a deprecation annotation to the old functions so that we see in the code when we use those functions?

@mojtaba-esk
Copy link
Contributor Author

Is it possible to add a deprecation annotation to the old functions so that we see in the code when we use those functions?

good idea, done.

tty47
tty47 previously approved these changes May 27, 2024
pkg/k8s/k8s.go Show resolved Hide resolved
MSevey
MSevey previously approved these changes May 28, 2024
Copy link
Member

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

lgtm. I think all open discussions can be turned into follow up issues 👍🏻

@mojtaba-esk mojtaba-esk dismissed stale reviews from MSevey and tty47 via dd1e91c May 28, 2024 16:43
@mojtaba-esk mojtaba-esk requested review from tty47 and MSevey May 28, 2024 16:44
Copy link
Contributor

@tty47 tty47 left a comment

Choose a reason for hiding this comment

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

you 🪨 🙌

@mojtaba-esk mojtaba-esk added this pull request to the merge queue May 29, 2024
Merged via the queue into main with commit 4efa658 May 29, 2024
3 of 4 checks passed
@mojtaba-esk mojtaba-esk deleted the mojtaba/refactor-knuu-pkg branch May 29, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

refactor!: have knuu as an object
4 participants