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 toString methods to disk and partition API #943

Merged
merged 5 commits into from
Aug 11, 2019

Conversation

agithyogendra
Copy link
Contributor

@agithyogendra agithyogendra commented Aug 11, 2019

HWDiskStore & HWPartition toString Methods for #942

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Looks great! Just a note, let's keep the indentation formatting and newlines in the "test" class and have the toString return a String without leading or trailing whitespace/newline/characters.

public String toString() {
boolean readwrite = getReads() > 0 || getWrites() > 0;
StringBuilder sb = new StringBuilder();
sb.append(' ').append(getName()).append(": ");
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave the space on the SystemInfoTest class and start this one with the name?

sb.append(" (").append(readwrite ? FormatUtil.formatBytes(getReadBytes()) : "?").append("), ");
sb.append("writes: ").append(readwrite ? getWrites() : "?");
sb.append(" (").append(readwrite ? FormatUtil.formatBytes(getWriteBytes()) : "?").append("), ");
sb.append("xfer: ").append(readwrite ? getTransferTime() : "?").append("\n");
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add the trailing new-line here.

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append(" |-- ").append(getIdentification()).append(": ");
Copy link
Member

Choose a reason for hiding this comment

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

Similar to earlier comment let's keep the indentation (" |-- ") in the SystemInfoTest output and start the toString with text.

sb.append("(").append(getType()).append(") ");
sb.append("Maj:Min=").append(getMajor()).append(":").append(getMinor()).append(", ");
sb.append("size: ").append(FormatUtil.formatBytesDecimal(getSize()));
sb.append(getMountPoint().isEmpty() ? "" : " @ " + getMountPoint()).append("\n");
Copy link
Member

Choose a reason for hiding this comment

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

And similar to earlier, don't include the newline in the toString. If needed for the output, add in the other class (but it may not be needed there).

@codecov-io
Copy link

codecov-io commented Aug 11, 2019

Codecov Report

Merging #943 into master will increase coverage by 0.28%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #943      +/-   ##
============================================
+ Coverage     74.85%   75.14%   +0.28%     
  Complexity        2        2              
============================================
  Files            22       22              
  Lines          1038     1058      +20     
  Branches        130      137       +7     
============================================
+ Hits            777      795      +18     
  Misses          204      204              
- Partials         57       59       +2
Impacted Files Coverage Δ Complexity Δ
...-core/src/main/java/oshi/hardware/HWPartition.java 78.94% <100%> (+1.93%) 0 <0> (ø) ⬇️
...-core/src/main/java/oshi/hardware/HWDiskStore.java 63.93% <83.33%> (+2.11%) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f64e485...496e574. Read the comment docs.

@coveralls
Copy link

coveralls commented Aug 11, 2019

Coverage Status

Coverage increased (+0.4%) to 78.355% when pulling 496e574 on agithyogendra:HWDiskStore_toString into f64e485 on oshi:master.

@dbwiddis dbwiddis changed the title Issue #942: Add toString methods to API interface implementations Add toString methods to disk and partition API Aug 11, 2019
Removed leading or trailing whitespace/newline/characters.
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Looks great!

@dbwiddis
Copy link
Member

Looks great! I tweaked some formatting, removed an unneeded null check, and added a test and changelog entry. Will merge as soon as the CI tests finish!

Thanks for contributing to open source!

@dbwiddis dbwiddis merged commit 534c11b into oshi:master Aug 11, 2019
@agithyogendra
Copy link
Contributor Author

Thanks for the opportunity!

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.

4 participants