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

DetectVM - Wrong comparison of result #908

Closed
jgueth opened this issue Jul 17, 2019 · 9 comments
Closed

DetectVM - Wrong comparison of result #908

jgueth opened this issue Jul 17, 2019 · 9 comments
Labels
confirmed bug Confirmed bugs. Highest priority to fix. first-timers-only Issues reserved for brand new GitHub users good first issue Good issues for new contributors to work on

Comments

@jgueth
Copy link

jgueth commented Jul 17, 2019

In the class DetectVM (package oshi.demo), the method findOuiByMacAddressIfPossible returns an empty string if no entry was found in the vmMacAddressOUI map. However, the result is checked to null (not empty String), which means that the method always ends after the MAC address check.

@dbwiddis
Copy link
Member

dbwiddis commented Jul 17, 2019

Not only the MAC check, but the first MAC check. Looks like the error was introduced in #872.

@dbwiddis dbwiddis added confirmed bug Confirmed bugs. Highest priority to fix. good first issue Good issues for new contributors to work on first-timers-only Issues reserved for brand new GitHub users labels Jul 17, 2019
@dbwiddis
Copy link
Member

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. :-) 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.
  • Find the DetectVM.java class. At line 87, the code only checks for a non null string. This is a bug, because the code should also check for an empty string.
  • Alter the code to additionally check for an empty String.
  • Fork the project, commit your changes to your fork, create a PR, and get it merged
  • Celebrate

@shannondavid
Copy link
Contributor

Hi @dbwiddis. I haven't contributed to open source before and would like to give it a go. Do you mind if I contribute?

@yzhang2907
Copy link
Contributor

yzhang2907 commented Jul 17, 2019

Hey @dbwiddis! It has been done, this is my first time contributing to OS besides adding my name in the first-contribution github tutorial. It is PR #909. Thank you!

@dbwiddis
Copy link
Member

Hi, sorry @shannondavid it looks like @yzhang2907 beat you to the punch here! I can suggest a slightly more challenging issue for you to work on if you'd still like to help!

@shannondavid
Copy link
Contributor

@dbwiddis I'd love to!

@dbwiddis
Copy link
Member

@shannondavid check out #877. This comment covers the basics of what to do, scroll down to the end for the better Windows solution using event log.

@dbwiddis
Copy link
Member

Fixed in #909

@shannondavid
Copy link
Contributor

got it! checking out #877 now

dbwiddis pushed a commit that referenced this issue Jul 18, 2019
* Fixed issue 908, now empty string also checked.

* Fixed issue 908, now empty string also checked.

* Update DetectVM.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed bug Confirmed bugs. Highest priority to fix. first-timers-only Issues reserved for brand new GitHub users good first issue Good issues for new contributors to work on
Projects
None yet
Development

No branches or pull requests

4 participants