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

Add '-version' as command line option #427

Merged
merged 3 commits into from
Dec 10, 2022
Merged

Conversation

rit-clone
Copy link
Contributor

@rit-clone rit-clone commented Dec 3, 2022

Summary

Fixed #424

Changes

Added additional command line option -v & --version to retrieve currently installed version of Polypheny-DB


ToDo

  • Verify design and implementation

@rit-clone
Copy link
Contributor Author

rit-clone commented Dec 3, 2022

@hennlo Can you please review this PR and approve running workflows?

@hennlo hennlo added A-db Area: DB S-in-progress Still working on this pull request S-waiting-on-review The pull request is ready to be reviewed labels Dec 4, 2022
@hennlo
Copy link
Member

hennlo commented Dec 6, 2022

@rit-clone Thanks for submitting the PR.
I've a few remarks that you could work on.

1.) Your original idea with the log.infois much cleaner to work with, therefore please refrain from using System.out
2.) You could also add a short option for querying the version, similiar to the help option you can do: -v | --version
3.) I'm afraid polyphenyDb.getClass().getPackage().getImplementationVersion() won't return Polypheny-DB's version but null.
You'll need to retrieve the version as specified in gradle.properties file. However you cannot read the file from file system and need to access the version internally.

@hennlo hennlo added S-waiting-on-author The pull request requires changes by the author and removed S-waiting-on-review The pull request is ready to be reviewed labels Dec 6, 2022
@rit-clone
Copy link
Contributor Author

rit-clone commented Dec 6, 2022

@hennlo I'll make the changes for log.info and adding -v option as well. For point no.3, since Implementation version is referred from gradle.properties to build.gradle (https://github.com/polypheny/Polypheny-DB/blob/master/build.gradle#L25) and that version is in turn referred to dbms package's build.gradle (https://github.com/polypheny/Polypheny-DB/blob/master/dbms/build.gradle#L248) , the implementation version gets set while building. It will be null if you run it in an IDE but if the jar is run, I see the version. Will this approach not work?
Screenshot 2022-12-06 at 4 04 49 PM

@hennlo
Copy link
Member

hennlo commented Dec 6, 2022

@rit-clone Good catch, you're absolutely right, my bad. 👍
Also I thought about the Log output and the longer I think about it I like your solution actually better.

So to summarize:
Only 2.) is open for this PR

@rit-clone
Copy link
Contributor Author

Made the required changes. @hennlo please review.

@hennlo hennlo added S-waiting-on-review The pull request is ready to be reviewed and removed S-waiting-on-author The pull request requires changes by the author S-in-progress Still working on this pull request labels Dec 8, 2022
@hennlo hennlo self-requested a review December 8, 2022 05:12
Copy link
Member

@hennlo hennlo left a comment

Choose a reason for hiding this comment

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

PR looks good.

@hennlo
Copy link
Member

hennlo commented Dec 10, 2022

Thx @rit-clone, for this PR!

@hennlo hennlo merged commit 1ef840a into polypheny:master Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Area: DB S-waiting-on-review The pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add -version as additional command line option
2 participants