Tweak statuses

[iHiD]

We're soon to add approve_with_comment to the website. Once we do that I don't see much point in having the two different approve statuses. I suggest we refactor to approve and disapprove. Thoughts?

[yawpitch]

That'd make sense to be if "approve" had a parameter for "is_optimal" or something similar.

[SleeplessByte]

We'll lose the ability to branch logic later; even though approve_with_comment can be determined by looking at... comment_count > 0. (Think about filtering, or statistics, etc. They all become harder without the distinct statuses)

[iHiD]

@yawpitch Does it need it? Or can we just leave comments blank. The website won't do anything different depending on whether it's optimal or not.

@SleeplessByte Are they distinct statuses though? Or are they just statuses we've invented for the convenience of being able to roll one thing out before the other?

[yawpitch]

The comment being blank isn’t quite enough, since the comment is currently optional with approve_as_optimal, mandatory with approve_with_comment. I can see plenty of situations in which an optimal solution might benefit from a comment, but still be optimal.

Plus I could see plenty of benefit of trolling these retroactively for stats.

Also, side note, but are we encoding anywhere what version of the analyzer approved / didn’t approve?

[iHiD]

The comment being blank isn’t quite enough, since the comment is currently optional with approve_as_optimal, mandatory with approve_with_comment.

The problem is that the definition of approve_as_optimal is "any approval without a comment`. If there's an optimal solution with a comment, then it's not optimal (by the existing definition).

I'm just not convinced that there's a real different between optimal and not-optimal in any useful way. At least, I can't think of one.

Also, side note, but are we encoding anywhere what version of the analyzer approved / didn’t approve?

No. See #47

[yawpitch]

That definition contradicts the Guidelines:

approve_as_optimal: MUST be a best solution, SHOULD be without comment

As for the difference, it’s mainly statistical. Plenty of things will get approved that are far from optimal, knowing how many meet the optimal threshold (and especially how many get there after how many rounds) would potentially be very helpful.

Like on Python for two-fer there are four potential “approve” solutions, only two of which are “optimal”.

Re #47, that conversation seems to have stalled without consensus; seems to me storing the version number is going to be quite important in the long run. If we all simply standardize on semver in the analysis.json it should be sufficient.

[tehsphinx]

At some point I plan to add the random tips we have in exalysis to the go-analyzer. Then a student will get a random tip on further resources as soon as they get an approval. That means that I will never approve without a comment any more. There will always be at least the random tip.

We will then not be able to determine approve_as_optimal from len(comments) == 0. And this will also not be in line with the definition we currently have -- or I always have to use approve_with_comment.

If we want to have statistics on approve_as_optimal vs approve_with_comment then we need to keep them. Then it would make sense to rename approve_with_comment to approve_with_suggestion and allow comments on approve_as_optimal.

If we don't need them for statistics, we can combine them in approve.

[ErikSchierboom]

I can see plenty of situations in which an optimal solution might benefit from a comment, but still be optimal.

Then it is not optimal in my mind :)

Personally, I think merging the two makes sense, as it maps most fundamentally to what is actually happening on the website. The distinction between approve as optimal and approve with comment will only be useful for us, maintainers I feel. As my currently C# analyzer conforms to len(comments) == 0 means approve_as_optimal, to me this change is perfectly fine.

I'm just not convinced that there's a real different between optimal and not-optimal in any useful way. At least, I can't think of one.

I can't think of one either.

[yawpitch]

Then it is not optimal in my mind :)

In Python both of the following are optimal:

def two_fer(name="you"):
    return "One for {}, one for me.".format(name)

def two_fer(name="you"):
    return f"One for {name}, one for me."

The first uses str.format and the comment might be to consider the use of f-strings as shown in the second. The second uses f-strintgs, but the comment might be to be wary of using them for any string that might need to be internationalized, since they're eagerly-evaluated.

Both are entirely optimal; in both cases the student benefits from a comment.

The distinction between approve as optimal and approve with comment will only be useful for us, maintainers

That's not obviously an argument against maintaining the distinction, since it does hold potential real benefit for the mentors / maintainers. I'm fine with merging the two approve_XYZ statuses into a single approve, but I still say there's value in being able to distinguish the optimal from the less-than-optimal.

[tehsphinx]

Another sample from Go where a approve_as_optimal solution gets a comment and therefore is currently not approved automatically:

// Package twofer contains sharing algorithms.
package twofer

// ShareWith shares with given name.
func ShareWith(name string) string {
	if name == "" {
		name = "you"
	}
	return "One for " + name + ", one for me."
}

This is a perfect solution for twofer in Go. Still we make a comment to point to the fmt package in case the student does not know about it. The fmt package would be used to do interpolation but in comparison is a lot slower as the format needs parsing. Students might know that and intentionally use the + operator to concatenate.

[iHiD]

There's a few different conversations here I think:

where a approve_as_optimal solution gets a comment and therefore is currently not approved automatically:

I'm talking about merging the statuses once approve_with_comment is honoured. Sorry if that wasn't clear.

Both are entirely optimal; in both cases the student benefits from a comment.

Yes, this is the same as Ruby, and so both would get a approve_with_comment.

At some point I plan to add the random tips we have in exalysis to the go-analyzer.

I love this, but I wouldn't want it to come via analyzers - I'd want us to implement another system for this that looked at which tips students had, and recommended accordingly. So I don't think this comes into this discussion

That definition contradicts the Guidelines:
approve_as_optimal: MUST be a best solution, SHOULD be without comment

Yes. But I defined the guidelines and am suggesting redefining the guidelines :)

If we want to have statistics on approve_as_optimal vs approve_with_comment then we need to keep them. Then it would make sense to rename approve_with_comment to approve_with_suggestion and allow comments on approve_as_optimal. If we don't need them for statistics, we can combine them in approve.

I think this is the key. We don't need statistics because as you both demonstrate with your examples, optimal solutions can have comments.

The reason we split these into two categories was so that we could implements one set of things (approving) without having to implement the other (comments). Now we're also adding comments, I don't see a value in keeping both. I don't see a situation where it's valuable for us to put everything into two buckets of "This felt optimal" and "This didn't". If we're looking at statistics etc then I'd have thought we can use the granularity of "which comments were given?" to work out the optimalness of the solution, without having a specific status for it?

[SleeplessByte]

I think one of the arguments to keep the two statusses is if we want to keep the interface as is but display tips differently from comments.

Does that make sense?

I think this is the key. We don't need statistics because as you both demonstrate with your examples, optimal solutions can have comments.

Well, if we keep them, we probably want to change approve_as_optimal's spec to be OKAY to have commentary.

On the other hand, we can change the spec and output format to have tips: or something similar to accomplish this. So nothing is really blocking us right now from merging the two approve statusses.

[iHiD]

That's a nice way of saying this @SleeplessByte. I like the idea of seperating out tips (but let's not do it yet! - babysteps :))

[SleeplessByte]

In that case, merging is fine. I can't think of a reason not to merge.

[tehsphinx]

@iHiD, @SleeplessByte: agree with removing the separation. Makes my code simpler 😄

[ErikSchierboom]

Makes my code simpler

Same here :) I'm also in favor.