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

Set a Maven Property indicating if a database reused or created. #35

Merged
merged 3 commits into from Jul 23, 2014

Conversation

Projects
None yet
4 participants
@ChristianRedl

ChristianRedl commented Jul 22, 2014

This feature can be useful to control the behaviour of subsequent Maven steps. For example, if a database is filled with default data, a user might want to skip this step if an existing database was reused. See “example-database-reuse.apt.vm” for an example.

@ChristianRedl

This comment has been minimized.

Show comment
Hide comment
@ChristianRedl

ChristianRedl Jul 22, 2014

Owner

Find bugs "qulice" about too many methods. I refactored the "isOpen" out of this into a static class "SocketHelper" as it is a different concern. This alone wasn't enough though. After evaluating the rest of the class, I decided to ignore FindBugs, as the alternative would have been some nasty refactoring for the sake of it. I hope that is okay with you?

Find bugs "qulice" about too many methods. I refactored the "isOpen" out of this into a static class "SocketHelper" as it is a different concern. This alone wasn't enough though. After evaluating the rest of the class, I decided to ignore FindBugs, as the alternative would have been some nasty refactoring for the sake of it. I hope that is okay with you?

@ChristianRedl

This comment has been minimized.

Show comment
Hide comment
@ChristianRedl

ChristianRedl Jul 22, 2014

Owner

As previously discussed, "clean" is not final. Given this new feature it would make a lot of sense if it would be. To do so the Instance class needs to be refactored to set the properties in a constructor (rather than start method), unless you see a better way of doing this?

This of course will touch most of the other classes. I am happy to do this if you agree that this is the right thing to do, but have not yet done it as I wanted to keep the original change in its own commit and check with you if you are happy with the idea?

Owner

ChristianRedl commented on 41f5ec8 Jul 22, 2014

As previously discussed, "clean" is not final. Given this new feature it would make a lot of sense if it would be. To do so the Instance class needs to be refactored to set the properties in a constructor (rather than start method), unless you see a better way of doing this?

This of course will touch most of the other classes. I am happy to do this if you agree that this is the right thing to do, but have not yet done it as I wanted to keep the original change in its own commit and check with you if you are happy with the idea?

@dmarkov

This comment has been minimized.

Show comment
Hide comment
@dmarkov

dmarkov Jul 22, 2014

Thanks, I'll ask someone to do a code review here, thanks for your pull request

dmarkov commented Jul 22, 2014

Thanks, I'll ask someone to do a code review here, thanks for your pull request

@dmarkov

This comment has been minimized.

Show comment
Hide comment
@dmarkov

dmarkov Jul 22, 2014

@yegor256 could you please review this pull request

dmarkov commented Jul 22, 2014

@yegor256 could you please review this pull request

@yegor256

This comment has been minimized.

Show comment
Hide comment
@yegor256

yegor256 Jul 22, 2014

Member

@ChristianRedl many thanks for the pull request, see my comments above

Member

yegor256 commented Jul 22, 2014

@ChristianRedl many thanks for the pull request, see my comments above

@ChristianRedl

This comment has been minimized.

Show comment
Hide comment
@ChristianRedl

ChristianRedl Jul 23, 2014

@yegor256 See my comment about "this.project == null" above. Besides that I adapted all the changes you suggested and just pushed them into my fork.

ChristianRedl commented Jul 23, 2014

@yegor256 See my comment about "this.project == null" above. Besides that I adapted all the changes you suggested and just pushed them into my fork.

@yegor256

This comment has been minimized.

Show comment
Hide comment
@yegor256

yegor256 Jul 23, 2014

Member

@rultor good to merge

Member

yegor256 commented Jul 23, 2014

@rultor good to merge

@rultor

This comment has been minimized.

Show comment
Hide comment
@rultor

rultor Jul 23, 2014

Contributor

@rultor good to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

Contributor

rultor commented Jul 23, 2014

@rultor good to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 5b45296 into jcabi:master Jul 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@rultor

This comment has been minimized.

Show comment
Hide comment
@rultor

rultor Jul 23, 2014

Contributor

@rultor good to merge

@yegor256 Done! FYI, the full log is here (took me 10min)

Contributor

rultor commented Jul 23, 2014

@rultor good to merge

@yegor256 Done! FYI, the full log is here (took me 10min)

@yegor256

This comment has been minimized.

Show comment
Hide comment
@yegor256

yegor256 Jul 23, 2014

Member

@ChristianRedl many thanks!

Member

yegor256 commented Jul 23, 2014

@ChristianRedl many thanks!

@ChristianRedl

This comment has been minimized.

Show comment
Hide comment
@ChristianRedl

ChristianRedl Jul 23, 2014

Thank you very much!

That is me all done, nothing more in my fork to come this way.

ChristianRedl commented Jul 23, 2014

Thank you very much!

That is me all done, nothing more in my fork to come this way.

@yegor256

This comment has been minimized.

Show comment
Hide comment
@yegor256

yegor256 Jul 23, 2014

Member

cool. version 0.8 is in Maven Central, with all your changes

Member

yegor256 commented Jul 23, 2014

cool. version 0.8 is in Maven Central, with all your changes

@dmarkov

This comment has been minimized.

Show comment
Hide comment
@dmarkov

dmarkov Jul 25, 2014

@yegor256 I just added 17 mins to your account, many thanks for your contribution..

dmarkov commented Jul 25, 2014

@yegor256 I just added 17 mins to your account, many thanks for your contribution..

@dmarkov

This comment has been minimized.

Show comment
Hide comment
@dmarkov

dmarkov Jul 25, 2014

@rultor please deploy

dmarkov commented Jul 25, 2014

@rultor please deploy

@rultor

This comment has been minimized.

Show comment
Hide comment
@rultor

rultor Jul 25, 2014

Contributor

@rultor please deploy

@dmarkov OK, I'll try to deploy now. You can check the progress here

Contributor

rultor commented Jul 25, 2014

@rultor please deploy

@dmarkov OK, I'll try to deploy now. You can check the progress here

@rultor

This comment has been minimized.

Show comment
Hide comment
@rultor

rultor Jul 25, 2014

Contributor

@rultor please deploy

@dmarkov Done! FYI, the full log is here (took me 14min)

Contributor

rultor commented Jul 25, 2014

@rultor please deploy

@dmarkov Done! FYI, the full log is here (took me 14min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment