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

🪲Only show programs that has been modified in Overview of programs per adventure #5162

Merged
merged 13 commits into from
Mar 3, 2024

Conversation

jpelay
Copy link
Member

@jpelay jpelay commented Feb 19, 2024

Modifies the way we choose if a program is suitable for having a tick mark in the Overview of programs per adventure table.

The process is basically this:

  1. Get all of the code snippets from the yamls. For this I made a simple loop that kind of parses the markdown code, if it finds 3 consecutive backticks and then 3 more, the text found in-between is the code for that snippet.
  2. For the teacher adventures, I use BeautifulSoup to parse the HTML code used to format the text in these adventures.
  3. After getting all of the relevant code, and removing newlines, I compare the code from the student program with each one of the snippets. If the difference to any of the snippets is less than 10 characters, I consider the snippet not to be worthy of reviewing.

Fixes #5161

How to test

  • Login as a teacher and go to Overview of program per adventure
  • Login as a student in other window and copy-paste one of the example's code into the editor, and run the program.
  • Check that the program isn't being shown for reviewing.
  • Now in the student's window, modify the program before running it.
  • Now the program should be available for reviewing.

@ghost
Copy link

ghost commented Feb 19, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@hasan-sh hasan-sh self-requested a review February 21, 2024 14:24
Copy link
Collaborator

@hasan-sh hasan-sh left a comment

Choose a reason for hiding this comment

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

Hi @jpelay , great work!! I had the following error with a teacher adventure, but after resetting local db, it worked again.

I just wanna share my thoughts on the code. All works fine and all for loops are plausible. Just the one that you go through each charecter within the code is a bit of a hassle! And if the code is long, you can imagine the time complexity of this nested for loop would grow exponentially! So, why not doing it with reg?

Another suggestion is about how you decide whether there's a difference! Your decision is based on the charecter level and if there's 10 chars different, then you say they can modify. What if they just changed the line orders without adding anything new for instance? Do you know https://docs.python.org/3/library/difflib.html? Especially the sequenceMatcher! This way instead of relying on chars being changed from position, we rely on similarity score!

@jpelay
Copy link
Member Author

jpelay commented Feb 26, 2024

I just wanna share my thoughts on the code. All works fine and all for loops are plausible. Just the one that you go through each charecter within the code is a bit of a hassle! And if the code is long, you can imagine the time complexity of this nested for loop would grow exponentially! So, why not doing it with reg?

Hi! I did it this way in case there was weird nesting inside the adventures, I did it this way to make sure we parse it properly.

Another suggestion is about how you decide whether there's a difference! Your decision is based on the charecter level and if there's 10 chars different, then you say they can modify. What if they just changed the line orders without adding anything new for instance? Do you know https://docs.python.org/3/library/difflib.html? Especially the sequenceMatcher! This way instead of relying on chars being changed from position, we rely on similarity score!

I think this a worth suggestion!!! I implemented it and turned to be a lot better that just doing char diffs! I put an arbitrary value of 0.95 that I don't know if is too strict or not. Perhaps should we include a clause that says that if the original snippet had a underscore and the student filled them, it should be saved?

@Felienne
Copy link
Member

I put an arbitrary value of 0.95 that I don't know if is too strict or not. Perhaps should we include a clause that says that if the original snippet had a underscore and the student filled them, it should be saved?

I like that idea, but I am not sure it is worth the hassle of implementing it. Shall we open an issue and discuss it separately? (we can ask a teacher too).

@Felienne
Copy link
Member

I put an arbitrary value of 0.95 that I don't know if is too strict or not. Perhaps should we include a clause that says that if the original snippet had a underscore and the student filled them, it should be saved?

I like that idea, but I am not sure it is worth the hassle of implementing it. Shall we open an issue and discuss it separately? (we can ask a teacher too).

We agreed in the meeting to try to implement the underscore check into this PR after all!

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

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

Can be merged if we add the underscore check

@jpelay
Copy link
Member Author

jpelay commented Feb 27, 2024

Ready for review again!

can_save = False
return can_save

def has_placeholder(code):
Copy link
Collaborator

Choose a reason for hiding this comment

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

your forgot self!

@@ -267,10 +268,14 @@ def is_program_modified(self, program, full_adventures, teacher_adventures):
can_save = True
for snippet in adventure_snippets:
seq_match = SequenceMatcher(None, snippet, student_code)
if seq_match.ratio() > 0.95:
# Allowing a difference of more than 10% or the student filled the placeholders
if seq_match.ratio() > 0.95 or (not self.has_placeholder(student_code) and self.has_placeholder(snippet)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

i tested it with the restaurant adventure in level 1, and i can't seem the sutdent's progress still! The function is returning False for both snippets!

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right!! It should work now!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool and it does:)

Copy link
Contributor

mergify bot commented Mar 3, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 015c8a0 into main Mar 3, 2024
11 checks passed
@mergify mergify bot deleted the adventures_save branch March 3, 2024 15:39
Copy link
Contributor

mergify bot commented Mar 3, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

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.

🪲 Only show saved programs in "Overview of programs per adventure" if the program has been modified
3 participants