dmitri.shuralyov.com/website/gido

matching by import path pattern misses many packages dmitri.shuralyov.com/website/gido#5

Closeddmitshur opened this issue 4 years ago
dmitshur commented 4 years ago ยท edited

When an issue or change has a path that doesn't match any existing directory, it normally gets assigned to the repository root or "other". Such issues and changes are missed when doing a query by import path pattern.

This happens because they're added using the original paths, without checking that those paths correspond to any existing directory:

if len(paths) > 0 {
	c.Title = ImportPathsToFullPrefix(paths) + title
	switch c.State {
	case change.OpenState:
		// TODO: paths may match zero existing directories,
		// then it can never be matched by an import path pattern.
		// Need to check if all paths are unrecognized, and handle that appropriately.
		openChanges = append(openChanges, ChangeAndPaths{c, paths})
	case change.ClosedState, change.MergedState:
		closedChanges = append(closedChanges, ChangeAndPaths{c, paths})
	}
}

For example, consider https://gochanges.org/golang.org/x/playground which currently shows 4 open CLs, 148 closed CL, and https://gochanges.org/golang.org/x/playground/... which currently matches 0 open CLs and 9 closed CLs (143 CLs are missing).

Write Preview Markdown
dmitshur commented 4 years ago

This is a valid issue, but the root cause analysis was incorrect.

This happens because they're added using the original paths, without checking that those paths correspond to any existing directory

That statement wasn't true. Only import paths that correspond to an existing directory are added to paths.

What was missing was when paths == 0 and root != "", the change needed to be added to openChanges and closedChanges:

// Populate openChanges and closedChanges.
// ...
if len(paths) == 0 && root != "" {
	// Consider this change that didn't match any existing directories
	// to belong to the repository root.
	switch c.State {
	case change.OpenState:
		openChanges = append(openChanges, ChangeAndPaths{c, []string{root}})
	case change.ClosedState, change.MergedState:
		closedChanges = append(closedChanges, ChangeAndPaths{c, []string{root}})
	}
}

Also, this affects changes only, not issues, because issues that don't match any existing directories get assigned to "other", which cannot be matched by an import path pattern.

Fixed in commit b36d6d3cb7ee809e6244a5f67a571a2d70f46b4a.

Write Preview Markdown
dmitshur closed this 4 years ago
to comment.