Skip to content

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.

3 participants