scionproto / scion

SCION Internet Architecture
https://scion.org
Apache License 2.0
401 stars 161 forks source link

details: nits from SCIONLab #4629

Closed juagargi closed 2 months ago

juagargi commented 2 months ago

While in the process of merging FABRID into SCIONLab, we have found these very small nits to probably be beneficial to merge upstream (here):

jiceatscion commented 2 months ago

This change is Reviewable

juagargi commented 2 months ago

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @juagargi)

_pkg/addr/fmt.go line 59 at r2 (raw file):_

// ParseFormattedAS parses an AS number that was formatted with the FormatAS
// function. The same options must be provided to successfully parse.
func ParseFormattedAS(as string, opts ...FormatOption) (AS, error) {

Is this not sufficient for your use case?

If not, what is blocking you from using this instead of the newly introduced function?

I'm tagging @marcodermatt so that he sees this PR and discussion. In my opinion, fmt.ParseFormattedAS should be enough, but I don't know enough about FABRID or the performance they would need to have a canonical answer ...

jeltevanbommel commented 2 months ago

pkg/addr/fmt.go line 59 at r2 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…
I'm tagging @marcodermatt so that he sees this PR and discussion. In my opinion, `fmt.ParseFormattedAS` should be enough, but I don't know enough about FABRID or the performance they would need to have a canonical answer ...

We will use fmt.ParseFormattedAS indeed, so the change is not necessary. Thanks @oncilla!

juagargi commented 2 months ago

Reverted changes to parseAS