Ability to use dupl as a library

[iwankgb]

For some historical reason golanci/golangci-lint uses for of mibk/dupl. I am in the process of moving golangci-lint away from forks towards upstream versions of all used linters. To achieve this we must be able to use dupl as a library and this PR introduces such an ability.

[mibk]

Hi, sorry for the delay.

To achieve this we must be able to use dupl as a library […]

I think that's a great idea and I'm open to make this module available as a library. I have a slight problem with the pull request: there's too many, too chaotic commits. (I understand there's a history behind the commit chain.) Would you be willing to collaborate with me on turning these commits into a reasonable commit chain? I've already merge one of your commits, the one fixing the unchecked error.

I want dupl to stay compatible as much as possible. So far, this concerned only the CLI binary itself. If we're about to make some library packages part of the public interface (thus covered by the compatibility rules), I want to make a clear distinction between those: all private packages should go under internal/, making the rest public.

As the first step, I'd like to define how exactly should the public API look like. I tried inferring the API from the client perspective from this code block, but I'm not sure that's all of it. Could you reply with your vision of how the public API should look like (something like a brief package API with types and function signatures)? The goal is to come up with a small public API I'd feel comfortable staying compatible with in the following years. Thank you!

[mibk]

To elaborate further, this is the API implemented in your fork (if I understood correctly):

package api

type Issue struct {
	From, To clones.Clone
}

func Run(files []string, threshold int) ([]Issue, error)

package clones

type ByNameAndLine []Clone

type Clone struct {
	Filename  string
	LineStart int
	LineEnd   int
	Fragment  []byte
}

func PrepareClonesInfo(fread ReadFile, dups [][]*syntax.Node) ([]Clone, error)

type ReadFile func(filename string) ([]byte, error)

It's questionable why e.g. ByNameAndLine or ReadFile are part of the interface. Also, I'd prefer the public interface to be a single package. There's probably no need to split them into two.

[iwankgb]

internal package sounds great :) I will find some time to work on the PR during the weekend.

[iwankgb]

@mibk does it look acceptable?

[mibk]

First let's discuss the package interface. Originally dupl was design to find clone groups, not necessarily pairs. Would that be a problem? I was thinking about names as well and this is I've come up with:

package clone

type Pos struct {
	Line   int
	Column int
}

type Clone struct {
	Filename string
	From, To Pos
}

type Group struct {
	Clones []Clone
}

// SearchFiles searches files for clone groups.
func SearchFiles(files []string, threshold int) ([]Group, error)

I named the package "clone" because "api" felt too vague. What do you think?

@mibk does it look acceptable?

I'd prefare there be no unnecessary changes, e.g. s/mibk/golangci/ and back, but please, let's talk first and implement second so we're not wasting each other's time. Thank you!

BTW I have also considered implementing dupl as Go Analyzer, which seems to be a more standard way of writing Go linters nowadays.

[iwankgb]

I think that we can swap

type Group struct {
  Clones []Clone
}

for

type Group []Clone

(Group would be a container for []Clones and nothing more).
I think that dropping Fragment from Clone is not a good idea as golangci-lint uses it to output meaningful error message.

[mibk]

First of all, sorry for my late response.

I think that we can swap

type Group struct {
  Clones []Clone
}

for

type Group []Clone

(Group would be a container for []Clones and nothing more).

That's true, but having the container gives us more power in the future should a new group field (in addition to Clones) be added.

I think that dropping Fragment from Clone is not a good idea as golangci-lint uses it to output meaningful error message.

One argument would be that the client can always obtain the fragment by reading the file at the specified position. I'm not entirely against keeping the Fragment field, though.

[SVilgelm]

@iwankgb Would it be possible to you to finish this work?

[iwankgb]

@SVilgelm yes.

[SVilgelm]

thank you!

[LarsArtmann]

Is this PR still relevant?