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 API interface implementations #942

Open
dbwiddis opened this issue Aug 10, 2019 · 30 comments

Comments

@dbwiddis
Copy link
Member

commented Aug 10, 2019

I'm marking this issue for first-timers-only. That means that I will only accept a PR for this one from someone who's never contributed to open source before. This one is easy (but don't make that statement make you feel bad if you have a hard time with it, there's more to contributing to open source than changing lines of code, especially if it's your first time). I'll hold your hand through this if you need me to. :-)

We'd like to add toString() methods to all of the API classes, with the implementation in the associated Abstract class for interfaces. This has been done for AbstractOperatingSystem, AbstractDisplay, AbstractOSVersionInfoEx, AbstractSoundCard, AbstractUsbDevice, AbstractCentralProcessor, and the class OSProcess

Similar implementations should be made for:

  • ComputerSystem
  • Baseboard
  • Firmware
  • GlobalMemory
  • VirtualMemory
  • HWDiskStore
  • HWPartition
  • NetworkIF
  • PowerSource
  • Sensors
  • OSFileStore
  • NetworkParams

Feel free to do one, or multiple implementations! Comment below which one(s) you are doing to avoid stepping on someone else's toes.

Here are the steps to get a PR merged here.

  • Read the CONTRIBUTING file for reference. I'll walk you through anything you don't understand.
  • Fork the project to your own GitHub account, make a local clone, and create a branch for your submission.
  • Look at how this class's information is displayed in the SystemInfoTest class. This is a suggested starting point for the toString() implementation.
  • For interfaces, create a toString() method in the Abstract class implementing the interface. See the above links for examples.
  • For classes, create the toString() method in the class.
  • Alter the code in the SystemInfoTest class to call the toString() method you just created.
  • Commit your changes, push to to your GitHub fork, and create a Pull Request
  • Respond to the usually minor comments on code review
  • Celebrate! 🎉

@dbwiddis dbwiddis added this to the 4.1.0 milestone Aug 10, 2019

@dbwiddis dbwiddis pinned this issue Aug 10, 2019

@phillips0616

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

I will work on the implementations for ComputerSystem and Baseboard.

@rohitkukreja1508

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

I will work on FirmWare,GlobalMemory and VirtualMemory implementations

@agithyogendra

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

I will work on HWDiskStore and HWPartition

@colinbobolin

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Hello, @dbwiddis I'd like to contribute and work on the PowerSource implementation. Thanks for marking this as first-timers-only!

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

My pleasure, @colinbobolin. After all, contributing here was my first commit and now look at me. :)

This is actually a perfect Issue for that. Lots of small, accessible tasks that I don't have to do, and we all help each other!

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

General feedback since I've made the same comments on every PR so far...

Please keep the toString() to without any leading or trailing whitespace/newlines. Newlines in the middle of the string are ok in limited cases.

In contrast, please keep "formatting logic" in the SystemInfoTest class. By this I mean indentation, newlines for iterations, etc.

@BilalAM

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Hey @dbwiddis , might I recommend that instead of manually creating toString() strings , why don't we just use the Apache commons toStringBuilder class ? It is designed specifically for this purpose . It will be consistent throughout .

http://commons.apache.org/proper/commons-lang/javadocs/api-3.9/org/apache/commons/lang3/builder/ToStringBuilder.html

Just a simple return toStringBuilder(this).append(field1).append(field2) and etc will return a nicely formatted toString()

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Thanks for the recommendation @BilalAM . I do understand the value of consistent toStrings and the code you've linked to seems like a good "default" given no overrides.

I'll note, however, that this "manually creating output" has always been there in the SystemInfoTest class as a "quick summary of values" and we're mostly just refactoring it in this case. Standard toStrings that just replicate the exact values from the getters aren't as useful, in my opinion.

The javadoc for toString says it's:

a concise but informative representation that is easy for a person to read.

I think between the two options below, the second one is more concise and easier to read:

  • availableMemory=15032385536, totalMemory=34359738368
  • Available: 14 GiB/32 GiB

OSHI's focus is on providing the specific values (including values in the millions and billions, or double-precision floating points) like the full memory values above, and allowing the user to format appropriately for their needs. The toStrings are mostly just for a nice visual output, rounding, trimming and formating with metric/binary, etc.. and are mostly a "nice to have" that may not need a standard format.

"Device Tree" based structures like USB devices connected by hubs, or hard drive/partition arrangements, simply don't display well on a default/standard toString.

Finally, I'm always hesitant to add dependencies, and really don't think we need one just for toStrings.

@GandalfTheJava

This comment has been minimized.

Copy link

commented Aug 15, 2019

I see multiple people have already commented before me, but are there any implementations yet to be done? I would love to have a go. Thanks!

@BooSandy1994

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

I want to work on Sensors. I have never done these things before. It's a first for me and I might really need a helping hand.

@johnaearley

This comment has been minimized.

Copy link

commented Aug 15, 2019

@dbwiddis Hello, I would like to work on NetworkIF and NetworkParams if still available.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

Looks like @johnaearley has clamed the two remaining Network tasks and @BooSandy1994 has Sensors. @GandalfTheJava can you please take the OSFileStore? Look at HWDiskStore as a sample!

@GandalfTheJava

This comment has been minimized.

Copy link

commented Aug 16, 2019

@dbwiddis I will get started on OSFileStore immediately! Thank you

@BooSandy1994

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Hi @dbwiddis , now it is showing that you shared the repo with me. I surfed around and found the Sensor.java file in the repo. Is that the file I have to make changes to? I mean what's the process?

I found that this Sensor.java is an interface.

I kind of realise that I have to fork the project and make necessary changes.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

Hi, @BooSandy1994. Here's a step by step.

  1. On the top right of this page is a "Fork" button. Click it. This will give you a copy of OSHI on your own github account.
  2. Make a local clone of your own repository. Not sure what OS you have, but you will need to install git there, and then use the command git clone https://github.com/BooSandy1994/oshi.git
  3. Create a branch to do your work on. git checkout -b sensors-tostring
  4. Start coding! Since Sensors is an interface, you need to put the toString method in the implementation. To avoid repeating it in every OS's Sensors class you want to put it in the AbstractSensors class. So put a toString() method right here. You'll add the /** {@inheritDoc} */ javadoc, and an @Override annotation, then the method. See the completed PRs above in this thread for examples.
  5. Find the output currently in the SystemInfoTest class. It's at line 248-250. Leave the "Sensors: " portion in this class, and just append sensors.toString() to it.
  6. Take the remaining three lines and merge them into a single String in the toString. I'd suggest creating a String Temperature=55°C, Fan Speeds=[1234, 1235], Voltage=3.4V which you can probably do with a single String.format(). If you want to keep the 3 lines separate as they are, just create a StringBuilder and append the existing output (but replace the newline %n with a comma+space.)
  7. Commit your changes and push them to your own branch.
  8. On your github account, find the branch (which should say it is "ahead of master by 1 commit") and create a pull request.
  9. Find the pull request number and edit the changelog to add yourself in the list. Then commit and push again.
@BooSandy1994

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Omg thank you very much ... I am on it.

@BooSandy1994

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

@dbwiddis I just added the toString method in the Abstract Sensor class. Do I need to delete the remaining 3 lines after tha Oshi.add("Sensors:" + sensors.toString()). I have merged the cpu temp, fan speed and cpu voltage in the toString method only.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

Yes, delete those lines.

@BooSandy1994

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Also when I read the CONTRIBUTING section written at the start of this thread..... it is mentioned something like to run mvn test.
When I run it in git bash it says mvn command not found.
Also how to know if the changes I made are working or not.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

You would have to install maven on your machine OR you can use the mvnw maven wrapper batch file or command. See the "Readme" on the project homepage for how to run it!

You can run the SystemInfoTest in your IDE to see if the output looks normal.

@BooSandy1994

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Finally I was able push the changes with the chnagelogs and the code.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

Great job, @BooSandy1994 ! I'll just make a few formatting tweaks and merge it!

@oshi oshi deleted a comment from BooSandy1994 Aug 18, 2019

@dbwiddis dbwiddis unpinned this issue Aug 18, 2019

@BooSandy1994

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

Thank you very much .. it was a great first experience @dbwiddis

@rsersh

This comment has been minimized.

Copy link

commented Aug 18, 2019

Hi @dbwiddis , I have scanned the thread and I think your topics have all been covered but in case I missed something I would love the opportunity to help. If not, there's always next time. TY for posting.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

Hi, @rsersh , you are correct that all the topics have been claimed on this issue, but if you'd like to contribute there are many other things that can be done. Let me know if you'd like to pitch in elsewhere!

@rsersh

This comment has been minimized.

Copy link

commented Aug 18, 2019

Thank you, @dbwiddis. Yes, I would very much like to provide some assist where needed.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

@rsersh Check out #954

@Praveen101997

This comment has been minimized.

Copy link

commented Aug 22, 2019

Hi @dbwiddis it is my first time for looking for open source projects, and I really want to do this.
If there any opensource issues in java related please guide me what and how to do.
So, I will be able to contribute.

@dbwiddis

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

Hi, @Praveen101997 ! Glad to know you want to help. I'm not sure there's anything right now that's not being worked on, but if you "watch" the project (click the link near the top of this page) you can see new issues when they pop up.

@Praveen101997

This comment has been minimized.

Copy link

commented Aug 22, 2019

Thanx for your reply
I will make sure to help in the coming time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.