Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a parser to parse the phy name for fixing issue #1048(Bugfix) #1099

Closed
wants to merge 1 commit into from

Conversation

rickwu666666
Copy link
Contributor

Description

Adding a parser to parse out the phy name as follows.

root@ubuntu:/home/ubuntu# iw phy
Wiphy phy0
	wiphy index: 0
	max # scan SSIDs: 20
	max scan IEs length: 365 bytes
root@ubuntu:/home/ubuntu# iw phy
Wiphy mwiphy1
	wiphy index: 1
	max # scan SSIDs: 10
	max scan IEs length: 256 bytes

Resolved issues

This change is trying to fix the issue.

Documentation

Tests

Side load result on the machine that phy name is mwiphy:
https://pastebin.canonical.com/p/3bC2YR4Zj3/

Side load result on the machine that phy name is phy:
https://pastebin.canonical.com/p/7qKNP77gBH/

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.64%. Comparing base (884c449) to head (fe02104).
Report is 311 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1099   +/-   ##
=======================================
  Coverage   41.64%   41.64%           
=======================================
  Files         340      340           
  Lines       37742    37742           
  Branches     6417     6417           
=======================================
  Hits        15716    15716           
  Misses      21378    21378           
  Partials      648      648           
Flag Coverage Δ
provider-resource 22.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

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

As usual, thanks @rickwu666666 for providing a clear explanation and test submissions for the different cases.

I think these Shell commands are quite horrible to be honest. They were not readable before your patch (thank you, regular expressions!), and they got even worse with your patch because the line is even longer 😃

On top of that, none of this is tested.

Let's rewrite this using Python. This will allow us to add unit tests and have a more solid case for the future.

If you want, we can set some time aside and do some pair programming to tackle this. Just ping me :)

@rickwu666666
Copy link
Contributor Author

@pieqq Thanks, and I think we can leverage Issac's PR#1175 in the future. So I guess we can close this for now.

@Hook25 Hook25 reopened this Jun 21, 2024
@fernando79513 fernando79513 assigned Hook25 and pieqq and unassigned Hook25 Jul 16, 2024
@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Jul 16, 2024
@rickwu666666 rickwu666666 marked this pull request as draft August 6, 2024 06:51
@rickwu666666
Copy link
Contributor Author

Close this PR since the branch is pull from my personal repo. Create another PR for this topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes The review has been completed but the PR is waiting for changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants