fix: Key construction in ExtractGoComments#112
fix: Key construction in ExtractGoComments#112candiduslynx wants to merge 1 commit intoinvopop:mainfrom
ExtractGoComments#112Conversation
|
@samlown I think this actually fixes the intended use of See cloudquery/cloudquery#14647 for usage example with both vendored code & path that includes |
|
Hi @samlown! Is there a chance to get this PR (and the other PRs I opened t this repo) reviewed & merged? |
| "go/doc" | ||
| "go/parser" | ||
| "go/token" | ||
| "golang.org/x/exp/maps" |
There was a problem hiding this comment.
I'm not sure that we want to be using experimental stuff...
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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. @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? |
|
@samlown any updates? |
|
@samlown I'm closing this PR for 2 reasons:
If you'll be available for review & merging please tag me here & I'll reopen from my own fork. |
The intended (from README.md) usage of
ExtractGoComments(base, path string, commentMap map[string]string) errortreats the params as following:base– package import pathpath– package location on the filesystemcommentMapoutputIf we pass
"../"aspathparam the code will fail to load comments properly because of thepath.Joinused on the unverified input. Moreover, if thepathis the absolute path, the code will join it withbaseso we end up with keysbase + "/" + absPath(cleaned in thepath.Cleanfunc, but still).This PR fixes the behavior in the following way:
pathtorootPathfor better understanding the semantics & not allowing name clashes in the funcrootvariable equal to the absolute path of therootPathlocationrootpathrootprefix of the path prior to joining with thebasevalue