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

Removes util package #223

Merged
merged 1 commit into from
May 23, 2023
Merged

Conversation

m1kola
Copy link
Member

@m1kola m1kola commented May 19, 2023

Description

util is too generic for a package name.

Let's try and avoid packages like utils, common, etc as packages like this tend to become dumping grounds.

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@tmshort
Copy link
Contributor

tmshort commented May 19, 2023

I'm going to whine about the length of the name (conditionsets vs util), perhaps abbreviate (within the code, not necessarily the directory name) to cond?

@m1kola
Copy link
Member Author

m1kola commented May 19, 2023

@tmshort previously it was imported as operatorutil which is 1 char longer than conditionsets. I would prefer conditionsets over cond as it is less ambiguous. But yeah...

There are only two hard things in Computer Science: cache invalidation and naming things.

😄

@tmshort
Copy link
Contributor

tmshort commented May 19, 2023

I meant in the code - not the directory name. I don't think conditionsets.ConditionType is any clearer than cond.ConditionType. shrug

@m1kola
Copy link
Member Author

m1kola commented May 19, 2023

@tmshort I was consindering calling it cond or condition, but this is not about condition itself. I don't know how to make it clear that this is not about condition but rather about a set/collection of condition types. And at the same time not introduce confusion with API package and other common abbreviations.

Overall - I don't like this thing. Especially the fact that we tightly coupled API (api/v1alpha1/operator_types.go) with an utility package which only used for tests.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2023
@tmshort
Copy link
Contributor

tmshort commented May 23, 2023

ping @m1kola for rebase?

`util` is too generic for a package name.

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2023
@m1kola m1kola requested a review from tmshort May 23, 2023 15:52
@m1kola
Copy link
Member Author

m1kola commented May 23, 2023

@tmshort rebased. Please take another look.

@tmshort tmshort merged commit d6aa5e9 into operator-framework:main May 23, 2023
5 checks passed
@m1kola m1kola deleted the conditions_sets branch May 23, 2023 22:33
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.

None yet

3 participants