Handle Quoted Paths - #56
Conversation
josharian
left a comment
There was a problem hiding this comment.
Thanks. Always a good sign when a change makes other fixes easier…
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
josharian
left a comment
There was a problem hiding this comment.
what do you think about these simplifcations? (i haven't confirmed they pass all the tests, including the new ones i suggested! apologies if i'm leading you astray...)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
EliSauder
commented
Dec 12, 2025
@josharian I was looking over the stripPaths test cases, why did you decide to have non-path characters terminate the path segment rather than just have them be invalid? I think it would make more sense for all of these to expect an error. {desc: "equals terminates path", input: "Iface[a/b=c.T]", want: "Iface[b=c.T]"},
// Exclamation mark is excluded - path ends at !
{desc: "exclamation terminates path", input: "Iface[a/b!c.T]", want: "Iface[b!c.T]"},
// Semicolon is excluded - path ends at ;
{desc: "semicolon terminates path", input: "Iface[a/b;c.T]", want: "Iface[b;c.T]"},
// Question mark is excluded
{desc: "question mark terminates path", input: "Iface[a/b?c.T]", want: "Iface[b?c.T]"},
// Colon is excluded
{desc: "colon terminates path", input: "Iface[a/b:c.T]", want: "Iface[b:c.T]"},
// Space terminates path (spec says "graphic chars without spaces")
{desc: "space terminates path", input: "Iface[a/b c.T]", want: "Iface[b c.T]"},
// Unicode replacement character U+FFFD is excluded
{desc: "replacement char terminates path", input: "Iface[a/b\uFFFDc.T]", want: "Iface[b\uFFFDc.T]"}, |
josharian
commented
Dec 12, 2025
Time box expired, decided to just ship, same as above. |
josharian
left a comment
There was a problem hiding this comment.
OK, basically there. bunch of little nits. if you want, say the word and i'll make the last few changes myself. otherwise, comments below. things that occur multiple places i've noted only once. oh, and please run gofumpt at the end.
| {input: "github.com/go-chi/chi.Router[github.com/some-org/pkg.SomeType]", path: "github.com/go-chi/chi", typ: Type{Name: "Router", Params: []string{"pkg.SomeType"}}}, | ||
| // Quoted path edge cases - unbalanced/double quotes | ||
| //// missing closing quote |
There was a problem hiding this comment.
Add a comment above these explaining why they are here-but-commented-out.
| return string(out) | ||
| // We want balanced quotes for our paths | ||
| if quotesRemoved % 2 != 0 { |
| return string(out), nil | ||
| } | ||
| func getNonPathSeg(runes []rune, quotesRemoved *int) (seg []rune, remain []rune, more bool) { |
There was a problem hiding this comment.
please return quotesRemoved instead of passing pointer. and it looks like it can be a bool (singular quoteRemoved), not an int, which makes it clearer.
| *quotesRemoved += lenPreTrim - len(seg) | ||
| // If there are no path like characters, we are done | ||
| if n == -1 { | ||
| return seg, []rune{}, false |
| return seg, remain, true | ||
| } | ||
| func getPathSeg(runes []rune) (seg []rune, remain []rune, more bool) { |
| return seg, remain, true | ||
| } | ||
| func checkForQuote(p []rune) bool { |
| return p[0] == '"' || p[len(p)-1] == '"' | ||
| } | ||
| func trimPathSeg(p []rune) []rune { |
| for len(remain) > 0 { | ||
| var seg []rune | ||
| var more bool |
There was a problem hiding this comment.
very optional: change more to done, because done is marginally nicer than !more as a condition.
EliSauder
commented
Dec 12, 2025
@josharian I realized, without handling those test cases for findInterfaces, this won't actually resolve #13. I don't think it makes sense to merge in without that. So, I'll take a look at it along with your comments. |
Since I was in here and the parsing is updated, I figured #13 probably wouldn't be that bad to add. Here is my go at it :)