search

sileht / bird-lg

bird looking glass
Other
315 stars 110 forks source link

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

Closed tamihiro closed 4 years ago

tamihiro commented 5 years ago

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)

tamihiro commented 5 years ago

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.

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 commented 5 years ago

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

zorun commented 5 years ago

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

tamihiro commented 5 years ago

@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 commented 5 years ago

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

tamihiro commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

zorun commented 4 years ago

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

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