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

Added toggle function to preamble functor #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

T-Brick
Copy link

@T-Brick T-Brick commented Feb 22, 2022

show : Rational.t -> bool which is used to determine whether the preamble should be included in the string. Useful for when you only want to show certain messages if a student doesn't get a high enough score.

@T-Brick T-Brick added the enhancement New feature or request label Feb 22, 2022
Copy link
Member

@HarrisonGrodin HarrisonGrodin left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this as-is, since it would break existing autograders using the Preamble functor (which we shouldn't do without a major-version bump). We could add a second functor, though (maybe ConditionalPreamble?) and then implement the existing Preamble in terms of it.

src/Preamble.fun Outdated
@@ -1,5 +1,6 @@
functor Preamble (
val preamble : string
val show : Rational.t -> bool
Copy link
Member

Choose a reason for hiding this comment

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

One thought: instead of Rational.t, what about Grader.Rubric.t, so the preamble switch doesn't have to factor through the score?

Upon further reflection, perhaps we should even generalize to val show : Grader.Rubric.t -> string option or something, allowing for a different preamble per rubric item? (The existing behavior could be recovered with Fn.const (SOME preambleString), of course.)

Copy link
Author

Choose a reason for hiding this comment

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

I don't think we should merge this as-is

Fair

val show : Grader.Rubric.t -> string option or something, allowing for a different preamble per rubric item?

I thought about this but the issue I see with it is that we need to know the what the rubric is which isn't exactly clear and would make it hard to actually use this.

@T-Brick T-Brick requested a review from HarrisonGrodin March 5, 2022 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants