search

patric-r / jvmtop

Java monitoring for the command-line, profiler included
GNU General Public License v2.0
1.22k stars 252 forks source link

Refactoring for use as an external library #74

Open f-lombardo opened 8 years ago

f-lombardo commented 8 years ago

I refactored jvmtop in order to separate logic from presentation. This way, jvmtop could be used both as a library and as a program.

patric-r commented 8 years ago

Wow, awesome. I'll have a detailed look within the next days.

patric-r commented 8 years ago

I'll took a first look on your PR. While I'm happy with most of your changes, I've got a few early questions/comments:

  1. Is there any reason why you removed the .settings directory?
  2. You used another code-formatting style which also makes the review more difficult. I'd really appreciate using the same style as the existing code does.
  3. pom.xml lacks unit-test definitions (junit dependency etc.)
  4. com.jvmtop.openjdk.tools.MemoryPoolStat.getAfterGcUsage() seems to be broken, it returns the same value like getBeforeGcUsage()
f-lombardo commented 8 years ago
  1. The .settings directory was automatically added to .gitignore by Eclipse
  2. It's OK to use another formatting style. If you like, please send my an Eclipse formatting style: I would apply it to the project.
  3. Fixed pom.xml
  4. Fixed bug in MemoryPoolStat

I pushed my changes. Should I send you another pull request?

patric-r commented 8 years ago
  1. Strange, my eclipse doesn't seem to do that. Can you recover .settings and remove .settings from .gitignore? Doing that will automatically apply correct formatting style to the project. You just have to reformat all sources - then everything should look perfect and diffing will just be painless.
  2. see above
  3. Ok, thanks, I'll have a look.
  4. Perfect.

After fixing issue 1. you can open another PR, yes. But I don't think this is necessary: when you add further commits to the corresponding branch they should automatically appear here in this pull request.

f-lombardo commented 8 years ago

I did it. Is it OK?

patric-r commented 8 years ago

Thanks so much. Please excuse that I still have some minor comments:

  1. According to the maven standard directory layout all resources which are required to build the project should be put below src. That's also true for the wrapper scripts jvmtop.sh and jvmtop.bat. If you don't like the current naming "wrappers" you can rename it to "scripts".
  2. jvmtop should still be compilable with java 1.6. Hence, please revert corresponding changes inside pom.xml and that files below `.settings'
  3. Is there any reason why you added those lines to .gitignore?
f-lombardo commented 8 years ago
  1. Done
  2. Done
  3. I don't remember
f-lombardo commented 8 years ago

Any news about this pull request?

Thanks.

Franco

mgrossmann commented 8 years ago

Please merge this pull request.

Thanks

aayoubi commented 7 years ago

This is great, @f-lombardo ! @patric-r will this get merged soon?

monkiki commented 7 years ago

Any update about this pull request?

f-lombardo commented 7 years ago

It seems that patric-r isn't an active github user anymore. What do you think about forking the project?

tckb commented 7 years ago

@f-lombardo yes, you are right. He hasn't been active for a while. I've been pseudo maintaining a fork of it, updating it a features as per my needs here. I've added Memory-profiler and added some more command line options for now. we can use this repo. @monkiki @aayoubi what do you say guys?

monkiki commented 7 years ago

I think it's the only option. Please, go ahead!