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

Refactoring for use as an external library #74

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

f-lombardo
Copy link

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
Copy link
Owner

patric-r commented Jan 5, 2016

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

@patric-r
Copy link
Owner

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
Copy link
Author

  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
Copy link
Owner

  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
Copy link
Author

I did it. Is it OK?

@patric-r
Copy link
Owner

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
Copy link
Author

  1. Done
  2. Done
  3. I don't remember

@f-lombardo
Copy link
Author

Any news about this pull request?

Thanks.

Franco

@mgrossmann
Copy link

Please merge this pull request.

Thanks

@aayoubi
Copy link

aayoubi commented May 8, 2017

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

@monkiki
Copy link

monkiki commented May 9, 2017

Any update about this pull request?

@f-lombardo
Copy link
Author

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

@tckb
Copy link

tckb commented May 9, 2017

@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
Copy link

monkiki commented May 10, 2017

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

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.

6 participants