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

Feature generalized tips #1072

Merged
merged 28 commits into from
Jun 7, 2018
Merged

Feature generalized tips #1072

merged 28 commits into from
Jun 7, 2018

Conversation

flbulgarelli
Copy link
Member

@flbulgarelli flbulgarelli commented May 27, 2018

Fixes mumuki/mumuki-platform#318
Fixes #952
Fixes #358

Supersedes #961

Screenshot pending

Final rules spec is here: mumuki/mumuki-platform#318 (comment)

end

def assistant
Mumukit::Assistant.parse(assistance_rules)
Copy link
Member Author

Choose a reason for hiding this comment

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

May cache this variable

middle_2: Also, %{tip}.
ending_0: Finally, %{tip}.
ending_1: Finally, %{tip}.
ending_2: Finally, %{tip}.
Copy link
Member Author

Choose a reason for hiding this comment

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

I need to fix those newlines.

middle_2: Also, %{tip}.
ending_0: Finally, %{tip}.
ending_1: Finally, %{tip}.
ending_2: Finally, %{tip}.
Copy link
Member Author

Choose a reason for hiding this comment

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

Also, a pt translation is missing


require_relative './assistant/rule'
require_relative './assistant/message'
require_relative './assistant/narrator'
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be extracted to a gem, so that is can be used in bibliotheca to validate the rules format.

Also, validation methods are missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

BibliothecaApi should use those methods

when :submission_errored then Mumukit::Assistant::Rule::SubmissionErrored.new(message)
when :submission_failed then Mumukit::Assistant::Rule::SubmissionFailed.new(message)
when :submission_passed_with_warnings then Mumukit::Assistant::Rule::SubmissionPassedWithWarnings.new(message)
else raise "Unsupported rule #{w}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Code may be simplified with a little metaprogramming

def matches?(submission)
submission.status.errored?
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be split into several files.

@@ -25,6 +25,10 @@
teacher_info: 'an info',
language: 'gobstones',
solution: 'foo',
assistance_rules: [
{when: :content_empty, then: 'remember to copy the code in the editor!'},
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to also add an editor for those rules in bibliotheca. Otherwise, this PR is pointless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@julian-berbel julian-berbel left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -27,8 +27,10 @@ def errored?
false
end

# False if and only if this status
# is `Mumuki::Laboratory::Status`
Copy link
Member

Choose a reason for hiding this comment

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

::Passed * ?

},
{
when: { error_contains: 'Unrecognized token %' },
then: 'Remeber you have to use `mod`, not `%`'
Copy link
Member

Choose a reason for hiding this comment

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

typo in Remember

@rules = rules
end

# Provides tips for the studen for the given submission,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@luchotc luchotc left a comment

Choose a reason for hiding this comment

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

👏 👏 👏

In some point would it be useful to add one that checks if the solution contains certain words? Like the common typos?

let(:tips) { [
'check you have not mispelled `product`',
'check that you are using composition',
'remeber that `sum` must work with both `Int`s and `Float`s'
Copy link
Contributor

Choose a reason for hiding this comment

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

remeber again

@@ -27,6 +27,14 @@ def corollary_box(with_corollary)
end
end

def assistance_box(assignment)
if assignment.tips.present?
%Q{<div class="mu-tips-box">
Copy link
Contributor

Choose a reason for hiding this comment

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

@julian-berbel missing indentation

@ghost ghost assigned julian-berbel Jun 7, 2018
@julian-berbel julian-berbel merged commit adbfbbf into master Jun 7, 2018
@ghost ghost removed the in progress label Jun 7, 2018
@flbulgarelli
Copy link
Member Author

In some point would it be useful to add one that checks if the solution contains certain words? Like the common typos?

I like that!

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

Successfully merging this pull request may close these issues.

3 participants