fix: Key construction in ExtractGoComments

[candiduslynx]

The intended (from README.md) usage of ExtractGoComments(base, path string, commentMap map[string]string) error treats the params as following:

• base – package import path
• path – package location on the filesystem
• commentMap output

If we pass "../" as path param the code will fail to load comments properly because of the path.Join used on the unverified input. Moreover, if the path is the absolute path, the code will join it with base so we end up with keys base + "/" + absPath (cleaned in the path.Clean func, but still).

This PR fixes the behavior in the following way:

• renaming path to rootPath for better understanding the semantics & not allowing name clashes in the func
• creating a root variable equal to the absolute path of the rootPath location
• walking the root path
• trimming the root prefix of the path prior to joining with the base value

[candiduslynx]

@samlown I think this actually fixes the intended use of AddGoComments as well (see the fixed test code as well).

See cloudquery/cloudquery#14647 for usage example with both vendored code & path that includes "../".

[candiduslynx]

Hi @samlown!

Is there a chance to get this PR (and the other PRs I opened t this repo) reviewed & merged?

[samlown]

I'm not sure that we want to be using experimental stuff...

[candiduslynx]

I can copy the implementation as it's just a straightforward map[key]value -> []value func.

LMK if you think this is the way.

[samlown]

If I understand correctly, this proposal will break previous implementations, certainly the tests have changed suggesting old code would need to be modified. I do not think thats a good idea unless absolutely necessary.

[candiduslynx]

If I understand correctly, this proposal will break previous implementations, certainly the tests have changed suggesting old code would need to be modified. I do not think thats a good idea unless absolutely necessary.

In this case it is just fixing the bug that exists in the mainline version.
It also allows for easier embedding of the cments from imported/censored structs.

@samlown as the test update suggests, the only issue seen here is adding the full pkg path for the import (or adding a root of vendored pkg to traverse correctly).

Could you please reassess this?

[candiduslynx]

@samlown any updates?