details: nits from SCIONLab

[juagargi]

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

• Allow the parsing of ASes from string using a different separator than :
• Improve the comment on the package variable HostAddr in package tools/integration.

[jiceatscion]

This change is 

[jiceatscion]

What is the benefit of supporting a separator other than ':'? In theory, I suppose it wouldn't hurt (it needs a test, though). But in principle it ads code for the sole purpose of supporting a notation that violates the spec we're in the process of submitting to the ietf. In that spec we're allowing only a narrow syntax. This is important in order to ensure that tools from various sources remain compatible in the long run. Can you imagine if every web browser on the planet had to support every possible notations for ip addresses that any prominent TCP/IP stack had once felt like promoting? This isn't yet a problem for SCION, but we should try and ensure it doesn't become one.

Reviewable status: 0 of 2 files reviewed, all discussions resolved

[oncilla]

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?

[juagargi]

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]

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]

Reverted changes to parseAS

[oncilla]

Reviewed 1 of 2 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @juagargi)

[oncilla]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @juagargi)