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

Processor identifier to centralProcessor #964

Merged
merged 7 commits into from
Aug 25, 2019

Conversation

praveenkumarsh
Copy link
Contributor

Move the ProcessorIdentifier inner class to the CentralProcessor class

@praveenkumarsh
Copy link
Contributor Author

Move the ProcessorIdentifier inner class to the CentralProcessor class
@dbwiddis
I commit the changes as per requirement
But not sure as it is my first contribution
If anything is wrong, please guide and correct me

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.

Great start! A few comments in line!


/**
* Name, eg. Intel(R) Core(TM)2 Duo CPU T7300 @ 2.00GHz
*
* @return Processor name.
*/
String getName();
@Deprecated String getName();
Copy link
Member

Choose a reason for hiding this comment

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

While technically correct, for style purposes put in a newline after the @Deprecated. Also add a @deprecated annotation to the javadoc (note the lower case) with a comment replaced by {@link ProcessorIdentifier#getName()}


/**
* Name, eg. Intel(R) Core(TM)2 Duo CPU T7300 @ 2.00GHz
*
* @return Processor name.
*/
String getName();
@Deprecated String getName();
Copy link
Member

Choose a reason for hiding this comment

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

These are valid, but for style purposes add a new line between the @Deprecated annotation and the get. You also need to add the (lowercase) @deprecated annotation in the Javadoc, pointing to the new location this information will be. Such as @deprecated Replaced by {@link ProcessorIdentifier#getName}

@dbwiddis
Copy link
Member

Also this needs a CHANGELOG entry.

@praveenkumarsh
Copy link
Contributor Author

Also this needs a CHANGELOG entry.

Do I have to also import this library in CentralProcessor which is previously imported in abstractCentralProcessor
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import oshi.util.ParseUtil;

@coveralls
Copy link

coveralls commented Aug 25, 2019

Coverage Status

Coverage increased (+0.2%) to 80.575% when pulling 0bbe0a6 on Praveen101997:processorIdentifier_to_Central into c0ae8a4 on oshi:master.

@codecov-io
Copy link

codecov-io commented Aug 25, 2019

Codecov Report

Merging #964 into master will increase coverage by 0.07%.
The diff coverage is 79.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #964      +/-   ##
============================================
+ Coverage     77.01%   77.09%   +0.07%     
  Complexity        2        2              
============================================
  Files            23       23              
  Lines          1114     1148      +34     
  Branches        140      143       +3     
============================================
+ Hits            858      885      +27     
- Misses          198      202       +4     
- Partials         58       61       +3
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/oshi/hardware/CentralProcessor.java 84.12% <79.41%> (-5.53%) 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 c0ae8a4...0bbe0a6. Read the comment docs.

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.

Fantastic job! This looks great, I'll do some formatting tweaks and merge it!

@praveenkumarsh
Copy link
Contributor Author

@dbwiddis I didn't add the issue name in the changelog.
So, I again pushed changes in it.
And the process is in way.

@dbwiddis dbwiddis merged commit c5b4f42 into oshi:master Aug 25, 2019
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.

None yet

4 participants