compose-spec / compose-go

Reference library for parsing and loading Compose YAML files
https://compose-spec.io
Apache License 2.0
350 stars 109 forks source link

Recursion in getStatementStart could be a loop #638

Closed panzi closed 1 week ago

panzi commented 3 months ago

Hey, I just noticed that the recursion here could be a simple loop, I think? (I'm new to Go, not new to programming.)

https://github.com/compose-spec/compose-go/blob/35c575c758afd2a8363bd47290c3ddec0d23ebaf/dotenv/parser.go#L77-L94

Could be something like this:

func (p *parser) getStatementStart(src string) string {
    for {
        pos := p.indexOfNonSpaceChar(src)
        if pos == -1 {
            return ""
        }

        src = src[pos:]
        if src[0] != charComment {
            return src
        }

        // skip comment section
        pos = strings.IndexFunc(src, isCharFunc('\n'))
        if pos == -1 {
            return ""
        }
        src = src[pos:]
    }
}

This is not a bug, just something I noticed while reading the source in order to understand exactly what syntax this tool is accepting. :smile: Sorry for the noise.

ndeloof commented 2 months ago

This code is inherited from https://github.com/joho/godotenv/blob/main/parser.go, there's no strong design decision from the compose-spec maintainers in this code.

Such "tail recursion" - AFAIK go compiler does not offer any sort of tail-Call optimization, which would magically detect and convert this into a loop - is generaly preferred to preserve readability vs nested loop (this is a short function, so not so obvious here)