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

Assessments housecleaning #51

Merged
merged 4 commits into from
Jun 18, 2018
Merged

Conversation

tuesmiddt
Copy link
Contributor

@tuesmiddt tuesmiddt commented Jun 17, 2018

  • Change Cadet context import to allow multiple imports
  • Minor cleanup of assessment imports
  • Removed unused aliases and removed some unused variables

- Allow using multiple contexts
- Cleaned up some imports in /lib/cadet/accessments
@coveralls
Copy link

coveralls commented Jun 17, 2018

Coverage Status

Coverage decreased (-0.02%) to 95.29% when pulling 3038187 on assessments_housecleaning into 2ef4900 on assessments.

Copy link
Member

@indocomsoft indocomsoft left a comment

Choose a reason for hiding this comment

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

LGTM save for for the single-line -> multiline definition

@@ -24,7 +18,9 @@ defmodule Cadet.Assessments do
alias Cadet.Assessments.QuestionTypes.MCQQuestion
alias Cadet.Assessments.QuestionTypes.ProgrammingQuestion

def all_missions, do: Repo.all(Mission)
def all_missions() do
Copy link
Member

Choose a reason for hiding this comment

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

I think the single-line definition was fine haha. Maybe you could share your rationale?

Copy link
Contributor Author

@tuesmiddt tuesmiddt Jun 17, 2018

Choose a reason for hiding this comment

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

It was more for consistency and clarity than anything else but I'm not too concerned either way. I doubt there will be many single-line functions in the code base. It's your call.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough

@indocomsoft indocomsoft added this to Under Review in Sprint 3 Jun 17, 2018
Copy link
Member

@indocomsoft indocomsoft left a comment

Choose a reason for hiding this comment

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

LGTM. Unless @sreycodes have any objection, I'll merge this tonight. Thank you for your work @tuesmiddt

@indocomsoft indocomsoft changed the title WIP: Assessments housecleaning Assessments housecleaning Jun 18, 2018
@indocomsoft indocomsoft merged commit 843bfa6 into assessments Jun 18, 2018
Sprint 3 automation moved this from Under Review to Done Jun 18, 2018
@tuesmiddt tuesmiddt deleted the assessments_housecleaning branch June 18, 2018 13:37
indocomsoft pushed a commit that referenced this pull request Jun 24, 2018
* Added Question and Assessment

* Added functionality for grading

* Added assessments functionality

* Fixed typo

* Restructured assessments

* Added assessments context

* Added controllers

* Delete duplicate file

* Changes to suit convention

* Fixed stuff

* Ran mix commands

* Fix changes

* Removed role validation

* Removed all compile time errors

* Fixed consistency issues

* Refactored

* Conversion to spaces

* More functionality for questions

* Adding tests

* Refactored

* Added few tests for missions

* Few more tests to go

* Added more tests

* Delete useless migration

* Delete useless migration 2

* Added submission test

* Added question tests and switched controllers to new branch

* Added answers test

* Remove unneccesary stuff

* Formatting

* Fixed compile error

* Make changes according to review

* Fixes

* Added tests to question and answer

* Added question types

* Various fixes

* Changes according to question types

* Assessments context

* Fixes

* Removed invalid test

* Fix some tests

* Assessments housecleaning (#51)

* Improve contexts

- Allow using multiple contexts
- Cleaned up some imports in /lib/cadet/accessments

* Raise exception when using context with invalid arguments

* More housecleaning

* Add comment to cadet context

* Fix credo

* Remove duplicate blank line

* Remove pre-commit

* Refactor json and rename Mission to Assessment

* Fix submission model

* Use cadet context

* Fix some json tests

* Fix tests and logic error

* Remove untested method and add one test

* Remove useless import

* Add test

* Fix according to code review

* Change to pipeline syntax

* Reorganise file
indocomsoft added a commit that referenced this pull request Aug 17, 2018
* Added Question and Assessment

* Added functionality for grading

* Added assessments functionality

* Fixed typo

* Restructured assessments

* Added assessments context

* Added controllers

* Delete duplicate file

* Changes to suit convention

* Fixed stuff

* Ran mix commands

* Fix changes

* Removed role validation

* Removed all compile time errors

* Fixed consistency issues

* Refactored

* Conversion to spaces

* More functionality for questions

* Adding tests

* Refactored

* Added few tests for missions

* Few more tests to go

* Added more tests

* Delete useless migration

* Delete useless migration 2

* Added submission test

* Added question tests and switched controllers to new branch

* Added answers test

* Remove unneccesary stuff

* Formatting

* Fixed compile error

* Make changes according to review

* Fixes

* Added tests to question and answer

* Added question types

* Various fixes

* Changes according to question types

* Assessments context

* Fixes

* Removed invalid test

* Fix some tests

* Assessments housecleaning (#51)

* Improve contexts

- Allow using multiple contexts
- Cleaned up some imports in /lib/cadet/accessments

* Raise exception when using context with invalid arguments

* More housecleaning

* Add comment to cadet context

* Fix credo

* Remove duplicate blank line

* Remove pre-commit

* Refactor json and rename Mission to Assessment

* Fix submission model

* Use cadet context

* Fix some json tests

* Fix tests and logic error

* Remove untested method and add one test

* Remove useless import

* Add test

* Fix according to code review

* Change to pipeline syntax

* Reorganise file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Sprint 3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants