otros-systems / otroslogviewer

Log viewer focused on developers work
Apache License 2.0
424 stars 87 forks source link

1.5.0 snapshot #601

Closed benlazaro closed 2 years ago

benlazaro commented 2 years ago

Hi Krzysztof, My name is Ben Lazaro and I was wondering if you're interested on someone to help you maintain and improve your project. I really like what you've done with OLV and I would like to contribute improving the code for maintainability and performance. I've done some changes to the project that I'm hoping we can consider them for the next minor version release. The changes are mostly about the gradle configuration (I updated the project to work with the latest gradle version 7.4.1), applying convention over configuration and stuff like that. These changes also include the creation of an executable uber-jar to replace the /libs folder in the distribution zip file. This simplifies the .bat and .sh file a little since we don't have to specify a classpath anymore. There are quite a few changes that I hope can be beneficial for the project. Please feel free to correct any possible mistakes I might have done trying to improve some files, especially the .bat and .sh files since I was trying to move as much of the configuration into the jar file (when applicable) so that in most cases it would be enough just to double click the .jar file to start up the application. Anyway, I look forward to hearing from you soon.

svennissel commented 2 years ago

I think you must check in the olv.sh and olv-batch.sh with execution rights. You can do this with following command:

git update-index --chmod=+x olv.sh
git update-index --chmod=+x olv-batch.sh

See stackoverflow.com updating-and-committing-only-a-files-permissions-using-git-version-control

But really great job. Works on Linux with intellij.

benlazaro commented 2 years ago

Noice!! that's awesome, thanks for letting me know, This is the first project where I use intelliJ for development. I've always used eclipse before. This is also my first project using gradle, and I must say I really like it. I think I'm eventually going to migrate our project at work from Maven to Gradle :) I'm really digging the incremental build, that's a huge time saver let me tell you. The execution rights for the .sh files is my fault. There is code in the olv-dist's build.gradle file that it's supposed to do that for us but after my changes I must have broke it. I'll fix it and I'll commit the changes.

benlazaro commented 2 years ago

Hey Sven, the execution rights for the .sh files should be fixed now. Now, let me know what you think about this... I would like these changes to go into a branch instead of merging them directly to master, so that we can (or myself if no one else has time for this) work on improvements for a future release of version 1.5.0. I did a quick search on how to merge the PR into a branch instead of master and I found this: https://stackoverflow.com/questions/9135913/merge-pull-request-to-a-different-branch-than-default-in-github If you're ok with this suggestion, could you create a branch in the repo so that this PR can be merged there instead of master?

svennissel commented 2 years ago

I test your fix, but i have no rights on this project. But I think it is OK to merge this in master.

benlazaro commented 2 years ago

My bad, I thought you were an admin for this project. Anyhow, I really appreciate your help testing out my fix. If everything looks good to you I will submit the PR for review and wait to see if it gets merge into master. This is the first time I'm contributing to an open source project here in github, so I'm learning as I go.

p91paul commented 2 years ago

@benlazaro GitHub marks with a small grey tag below the name as "Owner" or "Collaborator" people with commit rights to the repo. Sven is a "contributor" which means he had at least one pull request merged in the past, so he contributed code without being part of the actual project staff. As soon as this gets merged, you'll be a contributor too.

benlazaro commented 2 years ago

Hey @p91paul, thanks for taking the time to enlighten this lost soul of mine :) this makes sense and it’s easier to know what type of request/questions I can make to different people.

svennissel commented 2 years ago

I have create a branch gradle_update where i have fixed merge conflicts. I want to test it before i merge into master.