earthlab / abc-classroom

Tools to automate github classroom and autograding workflows
https://abc-classroom.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
29 stars 21 forks source link

Create move files helper function in utils module #292

Closed lwasser closed 3 years ago

lwasser commented 3 years ago

right now we copy files in three places:

  1. in template.py: https://github.com/earthlab/abc-classroom/blob/master/abcclassroom/template.py#L191
  2. template2 : https://github.com/earthlab/abc-classroom/blob/master/abcclassroom/template.py#L204
  3. clone.py: https://github.com/earthlab/abc-classroom/blob/master/abcclassroom/clone.py#L110
  4. feedback: https://github.com/earthlab/abc-classroom/blob/master/abcclassroom/feedback.py#L55

because it's in three places... with varying applications of checks it makes it harder to manage things like when it copies files that are hidden OS files and such.

it may be better to consider turningn this into a helper function https://github.com/earthlab/abc-classroom/pull/289/files#diff-17cca8b74aff25177c3008563a41d118R65 which could live in the utils module. i think this will be easier to maintain over time.

This helper would have to handle directories and also would skip moving files in the "ignored" list in the config. this should be easier to maintain because what has been happening is some of these errors get fixed in one place but not others which is problematic.

This also would make testing easier!! we can just test this specific helper

lwasser commented 3 years ago

ok @nkorinek @kcranston let's talk about moving files... above there are 4 places where we move files.

There are a few issues with moving file

  1. Sometimes files we don't want to move get moved (DS_Store for example, ipynb_checkpoints)
  2. Sometimes files we WANT to get moved don't get moved. Right now if a student has a .py script (module). it does not get moved to the submitted directory. And what if someone uses this for another programming language and has a js file for example.

In general i think there are also some system files we don't want to move around and that is why i implemented the files_to_ignore config option. Essentially this just alows a user to specify what they want to skip being moved.

How do we want to specify what types of files to move when? For instance - we may only want to move .py and ipynb files from the cloned_repo to submitted sometimes other times we may want an image(for a report) too. But if we move images then the images like the earthlab logo will also get moved. this could be ok?

Thoughts on this? @nkorinek does this sound inline with how you worked on that pr? Does your help consider files_To_ignore too?

lwasser commented 3 years ago

ok this move files thing i think we need to think through

here @nkorinek implemented copytree. this works well and copies the entire directory while ignoring files that you specify. the problem is that for some functions you only want to move files you plan to grade. I could see in some instances wanting to move images if the students are creating reports. we also would want to move .py scripts. there are also some instances when there is an html file that you do not want to move (if it's he graded submission).

for clone - the big issue is it moves the .git directory which we don't want... so all of a sudden my files_to_ignore list looks like this now to get somewhere close to what i want it to do:

files_to_ignore:
- .DS_Store
- ipynb_checkpoints
- colored-bar.png
- earth-lab-logo-rgb.png
- README.md
- .git

Originally i had envisioned manual copying and being able to select extensions that we want to move. Here you have to make sure everything else does not get moved. how do we want clone to behave? Do we only want to tell it what not to move or do we want to tell it what not to move and what to move? it would be easy to add a few default do not moves like .git. DS_store, checkpoints because i can't imagine anyone wanting to move those

clone -

What to move:

Skip (this works well with copy tree)


the html file is an edge case. it's the case where i've graded an assignment and have a feedback.html file in the repo for the student. by turning off "move to submitted" with a flag / parameter this issue can be resolved but i just wnat to think this through a bit more. the implementation is not what i had envisioned but i'm not sure what is best

i think what i am getting at here is do we move based on the file types we want to move only or based on what we want to ignore or both. Originally i had wanted both when i thought about this. but copytree does work nicely out of the box.

BTW - the above approach is going to fail for moving files in template because there we want to move images. so maybe i just answered my own question

another approach is teo

  1. implement files_to_move - this would only be for the clone --> submitted function really.
  2. files_to_ignore --> this would only be for hidden files that we don't want to move around

abc-clone: move only the file extensions listed in files_to_move ?? thhese are files needed for grading abc-template: move all files in the template dir - ignore hidden files and stuff listed in the files_to_ignore -- so this could easily use copytree feedback: move html files and ignore hidden files - so copy tree will work


so i think we can use copytree for

abc-template and feedback ** UPDATE - i looked at feedback.

clone will potentially need a different approach

lwasser commented 3 years ago

@kcranston i'm just looking for a second opinion here - any thoughts? ^^^ i think clone is the only spot where i just want to move notebooks and .py scripts and maybe images if students have reports

maybe we have files_to_grade that only gets used for abc-clone? that could actually work well. it would simplify everything and it could just be a list of extensions that we move ... so all .py and ipynb as a start. and that move happens manually

lwasser commented 3 years ago

i'm going to close this too. Essentially this got complicated because the move operations had different user requirements. so i ended up just handling each separately (i think). If it comes up again in the future we can revisit. closing for now..