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: remove getters in ClientSet #1646

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

jskelin
Copy link
Contributor

@jskelin jskelin commented Dec 9, 2024

Currently, all fields in ClientSet are public, and the getters don't contain any logic.

@jskelin jskelin self-assigned this Dec 9, 2024
@jskelin jskelin added the run-e2e-test Manually trigger the E2E tests for reviewed PRs label Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

Unit Test Results

1 911 tests  ±0   1 910 ✅ ±0   54s ⏱️ ±0s
  133 suites ±0       1 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 6599719. ± Comparison against base commit 23db28c.

♻️ This comment has been updated with latest results.

@jskelin jskelin changed the title chore: remove getters for ClientSet chore: remove absurd getters in ClientSet Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

E2E Test Results

    4 files  ±0    268 suites  ±0   39m 25s ⏱️ + 2m 24s
2 016 tests ±0  2 014 ✅ +1  2 💤 ±0  0 ❌  - 1 
2 131 runs  ±0  2 129 ✅ +1  2 💤 ±0  0 ❌  - 1 

Results for commit b286e88. ± Comparison against base commit 533fe77.

♻️ This comment has been updated with latest results.

@jskelin jskelin force-pushed the chore/delete_unused_getters branch from 8b95ab4 to b286e88 Compare December 9, 2024 10:15
@jskelin jskelin marked this pull request as ready for review December 9, 2024 10:15
@jskelin jskelin requested a review from a team as a code owner December 9, 2024 10:16
@jskelin jskelin added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels Dec 9, 2024
@jskelin jskelin force-pushed the chore/delete_unused_getters branch from b286e88 to 75acfde Compare December 10, 2024 07:42
@jskelin jskelin changed the title chore: remove absurd getters in ClientSet remove unreasonable getters in ClientSet Dec 10, 2024
@jskelin jskelin changed the title remove unreasonable getters in ClientSet remove getters in ClientSet Dec 10, 2024
@jskelin jskelin force-pushed the chore/delete_unused_getters branch from 75acfde to b832933 Compare December 10, 2024 09:36
Copy link
Contributor

@tomazpu tomazpu left a comment

Choose a reason for hiding this comment

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

When searching over the whole project i see that the code still uses .Settings(). Please also check for all other getters.

Edit: checked in a call was a false positive with not so optimal naming of mocks in tests.

@jskelin
Copy link
Contributor Author

jskelin commented Dec 10, 2024

When searching over the whole project i see that the code still uses .Settings(). Please also check for all other getters.

Edit: checked in a call was a false positive with not so optimal naming of mocks in tests.

There is no other appearance or method with the same name as the deleted getters

@jskelin jskelin requested a review from tomazpu December 10, 2024 13:26
@jskelin jskelin force-pushed the chore/delete_unused_getters branch from b832933 to 5e4b7ce Compare December 10, 2024 17:17
@jskelin jskelin force-pushed the chore/delete_unused_getters branch from 5e4b7ce to 6599719 Compare December 12, 2024 07:59
@jskelin jskelin changed the title remove getters in ClientSet refactor: remove getters in ClientSet Dec 16, 2024
@jskelin jskelin merged commit c7f0e9a into main Dec 16, 2024
12 checks passed
@jskelin jskelin deleted the chore/delete_unused_getters branch December 16, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-e2e-test Manually trigger the E2E tests for reviewed PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants