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 support for version arg #119

Merged
merged 3 commits into from
Dec 24, 2016
Merged

Add support for version arg #119

merged 3 commits into from
Dec 24, 2016

Conversation

jaa127
Copy link
Contributor

@jaa127 jaa127 commented Dec 20, 2016

This PR add support for reporting version info from apps (cli, gui). To support that, it refactors and cleans up cli Main-method vs. App logic. Now "extends App" is removed and it is plain object with main.

This also paves road for #115.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 68.879% when pulling a23390b on jaa127:version-arg into ceb7716 on hrj:master.

@hrj
Copy link
Owner

hrj commented Dec 20, 2016

I understand what you are doing here, but I think having two different versions (base version + app version) is complicating it too much and might lead to confusion.

And when making releases, in the foreseeable future, I am not planning to release app and base separately.

So, let's keep it simple for now. Only one version should be shown to users.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 68.966% when pulling 9029490 on jaa127:version-arg into ceb7716 on hrj:master.

@jaa127
Copy link
Contributor Author

jaa127 commented Dec 23, 2016

Hi, do you have any change to comment this or merge if this is ok? I have other stuff in pipeline, and this is kind of blocking that stuff. In anycase, happy holidays!

@hrj hrj merged commit 0aa0908 into hrj:master Dec 24, 2016
@hrj
Copy link
Owner

hrj commented Dec 24, 2016

I have merged this trusting that you have done the right thing. Happy holidays to you too 😄

I was not sure why the "throw" related changes were done. There was some problem in the past related to running the app and throwing of exceptions. But I don't recall now and I am travelling. So I am turning a blind eye on it 😉

@jaa127 jaa127 deleted the version-arg branch December 25, 2016 14:46
@jaa127
Copy link
Contributor Author

jaa127 commented Dec 25, 2016

Cool, thanks. It should be ok. Code was doing System.exit on that spot. This was not ideal, and there are already other exceptions passing through on that spot, so this new throw won't be a problem.

Moreover, App code is gone and it is plain object + main(args: Array): Unit. And in any case this is easier to handle than App logic (imho).

@hrj hrj modified the milestone: Next release Jan 27, 2017
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

3 participants