show route where net ~ [ prefix+ ] fails because + is not encoded

[tamihiro]

My PR #45 now includes patch 94d0357 for this glitch and related inconsistency in edge shapes of the map.
show route where net ~ [ 1.0.4.0/22+ ] (bgpmap)

• inconsistent without patch:
  

• consistent with patch:
  

[tamihiro]

Hi, there doesn't seems to be much reviewing going here, and I thought maybe the way I've opened PRs sporadically has made it difficult to follow.
I opened issues #43, #44, and #46, of which you've closed the first one, but one of the bugs reported in there hasn't been fixed.

• PR Correctly parse bird-2.0 output #42
  0261230 (unecessary now 2ce03eb has been committed to fix the same bug.)
  b6c46fc (referenced in issue AS path prepend count is incorrect in the map #44.)

• PR Correctly parse aspath for both bird 1.x and 2.0 #45
  5f98a35 (will fix an unresolved bug reported in issue bird-2.0.x compatibility #43.)
  94d0357 (referenced in show route where net ~ [ prefix+ ] fails because + is not encoded #46)

• PR Hyperlink ASN only when square-bracketed #47
  841df1e

I can close all these PRs and rebase the remaining 4 commits to my local master for which to open a new PR, if you think it'll be tidier and easy to review.
Pls let me know. I want to deploy this LG for my team, but don't want to maintain the code locally. Thanks.

[zorun]

Sorry for the delay in reviewing, I will have a look at these PRs soon.

[zorun]

@tamihiro Thanks for submitting these patches. Here is a quick review, I'm a bit confused:

• I don't understand 94d0357, it touches both request parsing/printing and graphical display code. Can you instead do two commits, each with a commit message that explains the problem and the solution?
• Hyperlink ASN only when square-bracketed #47 looks like it will introduce a regression. In show route for XXX all all AS numbers in BGP.as_path are clickable, is that still the case after your commit? What problem are you trying to solve?
• I still need to test AS path prepend count is incorrect in the map #44, I will try to do that at some point
• I think the remaining issue in bird-2.0.x compatibility #43 is now fixed thanks to Applying patch from @fduraffourg that fixes #9 #48, can you confirm?

[tamihiro]

@zorun Thanks for the reply. First let me answer your questions.

I don't understand 94d0357, it touches both request parsing/printing and graphical display code. Can you instead do two commits, each with a commit message that explains the problem and the solution?

These parsing and printing are pretty much coupled, but I'll see if I can try. Anyway please read on.

#47 looks like it will introduce a regression. In show route for XXX all all AS numbers in BGP.as_path are clickable, is that still the case after your commit?

Let me put it this way.
The output of show route for XXX all includes AS numbers:

[o] on the right side of BGP.as_path: that would match r'\d+', as you've suggested, and also
[o] on the via nexthop... lines, inside square brackets, that would match r'AS\d+'.

The output of show protocols <protocol_name> for all includes:

[o] AS numbers on the right side of Neighbor AS: that would match r'\d+', and also
[x] 4byte AS capability on the right side of Session: that would match r\bAS4\b'`.

What problem are you trying to solve?

The pattern in the last one marked with [x] should not be clickable, and I'm trying to solve it.

You can see it here for instance.
Here, AS4 should not be treated as ASN because it's about capability.

I think the remaining issue in #43 is now fixed thanks to #48, can you confirm?

Unfortunately not quite..

So.. at this point my PRs have made you confused, for which I'm sorry. Honestly I myself is confused too.
For clarity's sake, if you'd like, I'll be happy to drop all the PRs (#42, #45, #47) for now.
Then I'd pull your current master (#48 included), and on top of it write patches and resubmit a PR one by one.
Please consider and answer my suggestion at your earliest convenience. Thank you.

[zorun]

Thanks for the explanation, new PRs sound good! Make sure to include what you just explained in the commit messages :)

[tamihiro]

@zorun, I have dropped all the previous PRs, and resubmitted #49, #50, #51, #52, and #53.
Each one intends to correct an individual bug, so please review and confirm one by one. Each commit message hopefully gives you enough explanation.

[tamihiro]

@zorun it's been a while so I've just thought if there's anything that's still unclear. If there is pls feel free to ask.

[zorun]

@tamihiro yes, sorry for the delay, I have started reviewing the new PR!

[zorun]

It seems that some of your PR slipped through the cracks again :(

I have merged all of them, thanks again for your perseverance!