Skip to content

Add --afi filter option to report bgp command#2133

Merged
berendt merged 1 commit intomainfrom
filter-rep-bgp
Mar 14, 2026
Merged

Add --afi filter option to report bgp command#2133
berendt merged 1 commit intomainfrom
filter-rep-bgp

Conversation

@berendt
Copy link
Copy Markdown
Member

@berendt berendt commented Mar 14, 2026

AI-assisted: Claude Code

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • In the BGP AFI filtering loop, you recompute [a.lower() for a in parsed_args.afi] for every AFI; consider precomputing a lowercased set/list of AFIs once before the loop to avoid repeated work and make the intent clearer.
  • For the new --afi argument, you may want to leverage choices (or a small validator) in argparse to restrict values to known address families and catch invalid input early instead of silently filtering everything out.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In the BGP AFI filtering loop, you recompute `[a.lower() for a in parsed_args.afi]` for every AFI; consider precomputing a lowercased set/list of AFIs once before the loop to avoid repeated work and make the intent clearer.
- For the new `--afi` argument, you may want to leverage `choices` (or a small validator) in argparse to restrict values to known address families and catch invalid input early instead of silently filtering everything out.

## Individual Comments

### Comment 1
<location path="osism/commands/report.py" line_range="402-407" />
<code_context>
                 bgp_data = json.loads(bgp_result.stdout)

                 for afi, afi_data in bgp_data.items():
+                    if parsed_args.afi and afi.lower() not in [a.lower() for a in parsed_args.afi]:
+                        continue
                     peers = afi_data.get("peers", {})
</code_context>
<issue_to_address>
**suggestion:** Avoid recomputing the lowercased AFI list on every loop iteration by normalizing once up front.

This recreates a lowercased AFI list on every iteration of `bgp_data.items()`. To avoid repeated work and clarify intent, normalize the filter once before the loop, e.g.

```python
afi_filter = None
if parsed_args.afi:
    afi_filter = {a.lower() for a in parsed_args.afi}

for afi, afi_data in bgp_data.items():
    if afi_filter is not None and afi.lower() not in afi_filter:
        continue
    ...
```

This keeps normalization centralized and the loop leaner.

```suggestion
                bgp_data = json.loads(bgp_result.stdout)

                afi_filter = None
                if parsed_args.afi:
                    afi_filter = {a.lower() for a in parsed_args.afi}

                for afi, afi_data in bgp_data.items():
                    if afi_filter is not None and afi.lower() not in afi_filter:
                        continue
                    peers = afi_data.get("peers", {})
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread osism/commands/report.py
AI-assisted: Claude Code

Signed-off-by: Christian Berendt <berendt@osism.tech>
@berendt berendt merged commit 52e05a1 into main Mar 14, 2026
3 checks passed
@berendt berendt deleted the filter-rep-bgp branch March 14, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant