8000 Tidy up *dev status methods more by mulkieran · Pull Request #410 · stratis-storage/devicemapper-rs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Tidy up *dev status methods more #410

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Tidy up *dev status methods more #410

wants to merge 6 commits into from

Conversation

mulkieran
Copy link
Member
@mulkieran mulkieran commented Jan 29, 2019 •
edited
Loading

The objects of this PR are:

  • Better encapsulation.
  • To be better prepared for a possible future refactoring that eliminates the concept of distinct kinds of devices entirely.
  • To eliminate some needless boiler plate.
  • To get rid of the last risky assertion involved in reading a single status line.
  • It gets rid of some obsoleted comments.
  • It parse one last thinpool status value, held metadata root.

This is a continuation of #387 and really resolves #221.

@mulkieran mulkieran self-assigned this Jan 29, 2019
@mulkieran mulkieran changed the title Master status Tidy up *dev status methods more Jan 29, 2019
@mulkieran mulkieran force-pushed the master-status branch 3 times, most recently from 312b776 to 5298003 Compare January 29, 2019 21:41
@mulkieran
Copy link
Member Author

Should do a sanity check test for newly parsed thinpool status value.

@mulkieran
Copy link
Member Author

rebased.

@mulkieran mulkieran force-pushed the master-status branch 3 times, most recently from 89ca332 to 0e782ae Compare February 1, 2019 15:00
@mulkieran mulkieran requested review from tasleson and trgill February 1, 2019 15:28
@mulkieran
Copy link
Member Author
mulkieran commented Feb 1, 2019 •
edited
Loading

Uggg. I think I might have missed a spot. EDIT: Nope. That was in param parsing, not status parsing.

trgill
trgill previously approved these changes Feb 1, 2019
View reviewed changes
tasleson
tasleson reviewed Feb 4, 2019
View reviewed changes
src/shared.rs
format!(
"Insufficient number of fields for status; requires at least {}, found {}",
number_required, length
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be valuable from a debugging standpoint to include the status_line value in the error message?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would certainly be more complete.

@mulkieran
Implement FromStr for status structs
f538ecb 
It is better to have the implementation associated with the structure
that it builds rather than buried in the larger method. Also, this method
of getting something from a str is the expected way.

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran
Abstract checking number of fields into one method
4986076 
Eliminate the assertion and substitute an error return instead.

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran
Omit some obsoleted comments
82d143d 
The method no longer panics if there is an error parsing the status value.

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran
Also parse thinpool status value held metadata root
d0e13b5 
It has been omitted until now, because it seemed unclear how to parse it.
But it seems clearer now. Also, get rid of an obsoleted comment.

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran
Also abstract making an error if an unexpected value found
3d6b4d0 
This eliminates some unnecessary code duplication and fixes a
copy-paste bug.

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran
Include the actual status line in the error message
e890b58 
It could be helpful.

Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran mulkieran mentioned this pull request Feb 4, 2019
Merged
@mulkieran
Copy link
Member Author

Superceded by #417.

@mulkieran mulkieran closed this Feb 4, 2019
@mulkieran mulkieran deleted the master-status branch February 4, 2019 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tasleson tasleson tasleson approved these changes

@trgill trgill trgill left review comments

Assignees

@mulkieran mulkieran

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

expect! in status() methods will probably lead to embarassment eventually
3 participants
@mulkieran @tasleson @trgill
0