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

Modernise scan_disks() - no functional change intended #2826 #2827

Conversation

phillxnet
Copy link
Member

@phillxnet phillxnet commented Apr 3, 2024

By working directly with the Disk namedtuple during device collation, we gain readability and the advantages of an immutable type: i.e. we are then more explicit where writes/changes happen due to the requirement to use Disk._replace(). We also require less type transitions overall.

Includes:

  • Modernisation re type hints in scan_disks().
  • Improve lsblk line parser via modern Python builtins.
    Replaces by-hand lsblk output line parser with dictionary comprehension.
    Programmatically establishing lsblk field derived Disk namedtuple fields.
  • Refactoring for readability.
  • Employ defaults in Disk named tuple to ease creation using lsblk info, as these defaults pertain to our extra flag info.
  • Normalise scan_disks()'s working dict keys to lowercase. By normalising on lowercase we can ease the transition to using our Disk named tuple as the working copy: to help readability and highlight where we write (via Disk.replace(root=True)). This should also simplify the assembly of our final return value. We also have a large number of tests that use the existing lowercase Disk fields.
  • Update comments re now more readable code.
  • Black reformatting.

Fixes #2826

Includes:
- Modernisation re type hints in scan_disks().
- Improve lsblk line parser via modern Python builtins.
Replace by-hand lsblk output line parser with dictionary comprehension.
- Programmatically establish `lsblk` field derived Disk namedtuple fields.
- Refactoring for readability.
- Black reformatting.
- Employ defaults in Disk named tuple to ease creation using lsblk info,
as these defaults pertain to our extra flag info.
- Normalise scan_disks()'s working dict keys to lowercase.
By normalising on lowercase we can ease the transition to using
our Disk named tuple as the working copy: to help readability
and highlight where we write (via Disk.replace(root=True)).
This should also simplify the assembly of our final return value.
We also have a large number of tests that use the existing lowercase
Disk fields.

By working directly with the Disk namedtuple during device collation,
we gain readability and the advantages of an immutable type: i.e.
we are then more explicit where writes/changes happen due to requirement
to use Disk._replace(). We also require less type transitions overall.
@phillxnet
Copy link
Member Author

Testing

An rpm was successfully build with these changes, ensuring all existing tests passed. The resulting Rockstor instance worked as expected regarding disk presentation. And two example pools were successfully imported and interpreted.

N.B. this PR is a stepping stone to contain/present the interim no function change intended improvements as per the linked issue text.

@phillxnet
Copy link
Member Author

@FroggyFlox & @Hooverdan96 Merging as a dependency / prerequisite to my ongoing work towards:

As per linked issue text. This should then help with future assessments/understanding of why some changes were made to this rather sensitive and low-level piece of our puzzle.

@phillxnet phillxnet merged commit dbf5702 into rockstor:testing Apr 3, 2024
@phillxnet phillxnet deleted the 2826-Modernise-scan_disks()---no-functional-change-intended branch April 3, 2024 16:40
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