stl: improve error messages

[dlecorfec]

Encountered the following error message on a bad STL file:
astisub: building gsi block failed: astisub: atoi of � failed: strconv.Atoi: parsing "\x01": invalid syntax

Decided to replace the second occurence of "astisub" with the name of the target variable, in every place with int or date parsing, so this PR will output:
astisub: building gsi block failed: totalNumberOfDisks: atoi of failed: strconv.Atoi: parsing "\x01": invalid syntax
which makes it easier to pinpoint the bad location in the STL file.

(one could also wonder if it would be worth it to replace the %s of "atoi of %s failed" with a %q, in case of a non-printable string like in this example ...)

Bonus, line 835, removed the "astisub: " prefix which is redundant with the message line 206 in ReadFromSTL().

[asticode]

Adding the field targeted by the error is a good idea, however I'd rather keep the astisub: prefix which is there to indicate which package is submitting the error. Could you update error messages like astisub: totalNumberOfDisks atoi of %q failed: %w and revert the prefix you've removed line 835?

[dlecorfec]

Indeed, before submitting this PR, I checked that the astisub: prefix was already present in the public function (ReadFromSTL) of the package, which calls private functions (parseGSIBlock and newSTLCharacterHandler), where I removed the dups. So there's still an astisub: prefix (on line 199 and 206) indicating the package submitting the error.

[dlecorfec]

I added a change to ignore the Atoi errors when parsing TND and DSN: we encounter some STL with binary "1" instead of ASCII "1", and those values have no interests when convertings subs anyways (plus ttconv also ignores them).

[dlecorfec]

Like my previous change, ignore errors in Revision Number, which has no value when converting STL to another format.

[asticode]

Indeed, before submitting this PR, I checked that the astisub: prefix was already present in the public function (ReadFromSTL) of the package, which calls private functions (parseGSIBlock and newSTLCharacterHandler), where I removed the dups. So there's still an astisub: prefix (on line 199 and 206) indicating the package submitting the error.

I'd rather keep prefixing all errors with the package emitting the error.

[asticode]

Like my previous change, ignore errors in Revision Number, which has no value when converting STL to another format.

I'm wondering whether we should ignore all errors in parseGSIBlock. Something like:

	// Creation date
	if v := strings.TrimSpace(string(b[224:230])); len(v) > 0 {
		if v, err := time.Parse("060102", v); err == nil {
			g.creationDate = v
		}
	}

[dlecorfec]

Indeed, before submitting this PR, I checked that the astisub: prefix was already present in the public function (ReadFromSTL) of the package, which calls private functions (parseGSIBlock and newSTLCharacterHandler), where I removed the dups. So there's still an astisub: prefix (on line 199 and 206) indicating the package submitting the error.

I'd rather keep prefixing all errors with the package emitting the error.

I fully agree, but in main branch there's a double prefixing between caller and callee, so let's leave a single prefixing, here:

    // Parse GSI block
	var g *gsiBlock
	if g, err = parseGSIBlock(b); err != nil {
		err = fmt.Errorf("astisub: building gsi block failed: %w", err)
		return
	}

	// Create character handler
	var ch *stlCharacterHandler
	if ch, err = newSTLCharacterHandler(g.characterCodeTableNumber); err != nil {
		err = fmt.Errorf("astisub: creating stl character handler failed: %w", err)
		return
	}

[asticode]

Indeed, before submitting this PR, I checked that the astisub: prefix was already present in the public function (ReadFromSTL) of the package, which calls private functions (parseGSIBlock and newSTLCharacterHandler), where I removed the dups. So there's still an astisub: prefix (on line 199 and 206) indicating the package submitting the error.

I'd rather keep prefixing all errors with the package emitting the error.

I fully agree, but in main branch there's a double prefixing between caller and callee, so let's leave a single prefixing, here:

    // Parse GSI block
	var g *gsiBlock
	if g, err = parseGSIBlock(b); err != nil {
		err = fmt.Errorf("astisub: building gsi block failed: %w", err)
		return
	}

	// Create character handler
	var ch *stlCharacterHandler
	if ch, err = newSTLCharacterHandler(g.characterCodeTableNumber); err != nil {
		err = fmt.Errorf("astisub: creating stl character handler failed: %w", err)
		return
	}

parseGSIBlock may be called somewhere else at some point (in tests for instance) therefore I'd rather keep the prefix. But I don't think it matters anymore since with this parseGSIBlock won't return an error anymore 👍

[dlecorfec]

Ah, forgot about tests. Well, ignoring all GSI parsing errors doesn't seem such a good idea, some values are used afterwards.
I will restore the prefix in the error messages.

[asticode]

Thanks 👍

You just forgot to revert error prefixes in func parseDurationSTL() 👍

[asticode]

Thanks for the PR ❤️

Let me know whether you need a tag 👍

[dlecorfec]

Hi, a tag is welcome :)

[asticode]

FYI I've created the v0.37.0 tag 👍