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

Portion of refactoring and adjustments: #1

Merged
merged 1 commit into from May 22, 2016

Conversation

Projects
None yet
2 participants
@dustalov

This comment has been minimized.

Copy link
Member

dustalov commented May 18, 2016

Hello, thank you for your contribution! This PR seems to contain a lot of improvements, including better stage initialization mechanism, which is important for the users. However, it introduces several substantial changes, which I need to review thoroughly. At the present moment, I have a few suggestions to improve this contribution.

Firstly, it is mandatory to make the tests passing. Mechanical Tsar benefits a lot of Travis CI integration while the contributed code in its current state causes the tests to fail: https://travis-ci.org/mtsar/mtsar/builds/131091250. You could consider the .travis.yml file and run the test suite on your own computer. For example, check out my local test.yml.

Secondly, using the GET method for creating resources violates the REST style. Please use POST /stages for creating new stages, and PATCH /stages/:stage for updating them. The parameters should be passed as a form.

Thirdly, although, I have not reviewed the entire PR yet, I am not sure it is necessary to substitute all the Stage invocations with Stage.Definition. In the processors, we need to access the corresponding Stage instance, for which the Provider<Stage> approach has been used.

Lastly, please ensure the code is formatted as the IntelliJ IDEA does, and also run a static code analysis tool like mvn findbugs:gui to avoid potential bugs. Please also squash the commits into one when submitting an improved version—I will be very happy to accept your code!

@georgeee

This comment has been minimized.

Copy link
Contributor Author

georgeee commented May 21, 2016

Hello!

Done everything you've said except for last one - don't really know how to easily shrink already existing commits.

Please check

@dustalov

This comment has been minimized.

Copy link
Member

dustalov commented May 21, 2016

Great job! Currently, I do not understand one thing. For instance, in KOSAggregator now it appears two constructors: one accepts three arguments (stage and DAOs), another accepts two arguments (only DAOs). Why do we need both? Could we leave only one?

Well, it is possible to squash the existing commits using rebase and forced push. Since that you will push to your own fork, it will not break anything (and nobody will notice ☺). Merging this PR will not include the commits avoided by the forced push.

Portion of refactoring and adjustments:
* Made stages map synschronized with DB
* Added opportunity to add new/edit stage (via API)

Portion of refactoring and adjustments:
* Made stages map synschronized with DB
* Added opportunity to add new stage (via API):
  http://localhost:8080/stages/new?id=testStage1&workerRanker=mtsar.processors.meta.ZenCrowd&taskAllocator=mtsar.processors.task.RandomAllocator&answerAggregator=mtsar.processors.answer.MajorityVoting&description=test

Added opportunity to update stage

Refactored code to fix tests, injection

@georgeee georgeee force-pushed the georgeee:develop branch from 2e18209 to 93a6c46 May 21, 2016

@georgeee

This comment has been minimized.

Copy link
Contributor Author

georgeee commented May 21, 2016

Great job! Currently, I do not understand one thing. For instance, in KOSAggregator now it appears two constructors: one accepts three arguments (stage and DAOs), another accepts two arguments (only DAOs). Why do we need both? Could we leave only one?

It's being used in tests (to not to bother with injection there)

@georgeee

This comment has been minimized.

Copy link
Contributor Author

georgeee commented May 21, 2016

Done squashing into single commit

@dustalov

This comment has been minimized.

Copy link
Member

dustalov commented May 21, 2016

Got it. I will review the changes today again. In case everything is fine, this pull request will be merged no later than tomorrow's morning.

@dustalov dustalov merged commit c19616c into mtsar:develop May 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment