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 selector with feedback from 3696 #3704

Merged
merged 2 commits into from
Feb 20, 2023

Conversation

evankanderson
Copy link
Member

I decided to follow up on my comments on #3696 for filtering Buoy modules.

I figured writing the refactor was the easiest way to communicate my ideas; this removes the need for MatcherFunc and simplifies the calling inferface for Module and Modules in a few places. I'm considering replacing Selector with 'In()andExcept, but I'm not sure it is a substantial improvement. (Excludesis only used in one place, andIn(string...)andExcept(Matcher, Matcher)` would cover the same cases.)

/assign @cardil

@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/tools Issues or PRs related to internal tools found at /tools labels Feb 3, 2023
Copy link

@knative-prow knative-prow bot left a comment

Choose a reason for hiding this comment

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

@evankanderson: 0 warnings.

In response to this:

I decided to follow up on my comments on #3696 for filtering Buoy modules.

I figured writing the refactor was the easiest way to communicate my ideas; this removes the need for MatcherFunc and simplifies the calling inferface for Module and Modules in a few places. I'm considering replacing Selector with 'In()andExcept, but I'm not sure it is a substantial improvement. (Excludesis only used in one place, andIn(string...)andExcept(Matcher, Matcher)` would cover the same cases.)

/assign @cardil

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/gomod/selector.go Show resolved Hide resolved
@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 3, 2023
@krsna-m
Copy link
Contributor

krsna-m commented Feb 7, 2023

@cardil looks good to you?

…dule. Replaced it with a simpler implementation.
@evankanderson
Copy link
Member Author

I think Chris is only on mobile this week, according to another message on Slack.

This is anti-urgent, so I think it can wait.

I simplified the code one more time, removing Selector as an exported type in favor of returning an anonymous function which captures the "module under prefix but not in my current workspace" requirement.

func CurrentModulesMatcher() (Matcher, error) {
os := gowork.RealSystem{}
knownMods := sets.NewString()
mods, err := gowork.List(os, os)
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said in #3696 such refactor will remove the laziness, which was being done using sync.Once.

Now all work is done in factory function CurrentModulesMatcher() (Matcher, error), which is undesired, and unexpected behavior. Factory function shouldn't do a real costly work, especially if it isn't required.

If you really like to pursuit this refactor, I would advise changing the Matcher func to be:

type Matcher func(modname string) (bool, error)

That would allow moving the "work" to the interior of the matcher func, keeping the current lazy behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we taking advantage of that laziness anywhere?

The advantage to doing the work up-front is that Matcher can have a simpler signature that doesn't need to report errors, as we do the possibly-error-creating work up front. As far as I can tell, CurrentModulesMatcher is actually only ever used in this file, so we could even avoid exporting it at all.

(Yes, we could make this lazy, but I'm concerned about deferring the error to a place where it's more difficult to handle.)

Copy link
Contributor

@krsna-m krsna-m Feb 13, 2023

Choose a reason for hiding this comment

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

Take this with a grain of salt because of my ignorance of golang best practices and all, but for me readability (simpler signatures etc) > almost anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I think I'm convinced by you both. This lazy behavior isn't needed. It could be easily, re-introduced if it became a problem.

Also, removal of this log.Fatal is a great idea.

return func(string) bool { return true }, ErrNoDomain
}

currentModules, err := CurrentModulesMatcher()
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling DefaultSelector(domain string) (Matcher, error) will also do "the work", of looking for project modules, regardless if the given module matches the given domain name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it really important to defer the error handling into late in the game? It looked to me that we were only using DefaultSelector in the RunE commands for check, float, and needs, each of which invokes the DefaultSelector once.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -15,90 +14,49 @@ var (
)

// Matcher matches a module name.
type Matcher interface {
Copy link
Contributor

@cardil cardil Feb 15, 2023

Choose a reason for hiding this comment

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

I would really like to keep at least this interface.

It allows people passing not only functions, but compatible structs as well.

It can be easily implemented by simple func:

type MatcherFn func(modname string) bool

func (fn MatcherFn) Match(modname string) bool {
  return fn(modname)
}

and to return it:

return MatcherFn(func(modname string) bool {
  return strings.HasPrefix(modname, domain) && !currentModules(modname)
})

Copy link
Contributor

Choose a reason for hiding this comment

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

But, if you don't feel it is worth it, I can give LGTM right away.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you see how I used StringSet.Has as a Matcher below on line 41?

func CurrentModulesMatcher() (Matcher, error) {
  knownMods := sets.NewString()
  // ...
  return knownMods.Has, nil
}

I'm not convinced we need to declare an interface to define an interface (Matcher as a type alias for func(string) bool).

Copy link
Member Author

Choose a reason for hiding this comment

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

So in your example above, you could have:

  return MatcherFn(myMatcher).Match

Copy link
Contributor

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2023
@knative-prow
Copy link

knative-prow bot commented Feb 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, evankanderson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [cardil,evankanderson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1 similar comment
@knative-prow
Copy link

knative-prow bot commented Feb 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, evankanderson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [cardil,evankanderson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot merged commit 8e42b24 into knative:main Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/tools Issues or PRs related to internal tools found at /tools lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants