fix(reset): Improve cycle detector #709
Conversation
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
| // areInDifferentServices checks if two paths are in different service definitions | ||
| func areInDifferentServices(path1, path2 string) bool { | ||
| // Split paths into components | ||
| parts1 := strings.Split(path1, ".") |
There was a problem hiding this comment.
nit: could use path.Parts() if you don't convert it to a string
There was a problem hiding this comment.
the function accepts strings and the field also has a list of strings visitedNodes map[*yaml.Node][]string, do you suggest to use path everywhere? or to convert the strings inside the func into paths and use Parts()? it looks like overhead, isn't it?
There was a problem hiding this comment.
some overhead indeed, but my point is that Path abstraction allows to ignore "implementation details" like the dot notation and [] or * patterns. I had in mind at some point we might want to use an alternate implementation vs string as we actually have to split this string many times during parsing.
From an encapsulation point of view, I'd prefer we add more utility func to Path to support required submatching.
ANYWAY I'm fine we merge this PR as a quick fix for this issue, and revisit this later
There was a problem hiding this comment.
@ndeloof agree, that sounds reasonable.
Improved the cycle detector and added one more test case.
docker/compose#12247 (comment)