rundel / ghclass

Tools for managing classroom organizations
https://rundel.github.io/ghclass/
GNU General Public License v3.0
142 stars 22 forks source link

Repo add file enhancement #53

Closed thereseanders closed 5 years ago

thereseanders commented 5 years ago

This PR conducts a number of changes on repo_add_file()`

thereseanders commented 5 years ago

Changed the check_file_modification test to file_exists. The current form of check_file_modification will return TRUE on deleted files and won't allow to add new files. This causes functions calling repo_add_file to fail.

We should talk about whether we want to include the check_file_modification again that checks for student versus admin modifications.

thereseanders commented 5 years ago

Based on our conversation, I implemented the following:

  1. Changed file_exists() to file_exists_current()
  2. Wrote new function file_exists() that uses check_file_modification() to see whether file ever exists, and only if TRUE is returned uses file_exists_current() (that is costly in terms of calls to the API).

Currently not implemented

thereseanders commented 5 years ago

I made more modifications on repo_add_file() and helper functions.

  1. Changed file_exists() to file_exists_current()
  2. repo_add_file() now calls check_file_modification() as part of the commit messaging.
  3. added new function check_file_everexist() that checks for a length of the output of get_committer() > 0.
  4. Only if check_file_everexist() evaluates to true is the costly call to file_exists_current() implemented. We should think about whether to replace this functionality with a GET call instead.
  5. I will use a purrr::safely(github_api_repo_get_file)() call in the peer review functions whenever possible to reduce calls to check_file_everexist().
thereseanders commented 5 years ago

In the new version, PR makes the following changes:

rundel commented 5 years ago

Looks good, I made a slight simplifying change to use fs instead of a regex to prepend the folder to the repo path.

rundel commented 5 years ago

Only small concern I have is that I found the folder parameter name slightly confusing without reading the docs. Since this refers to something that will happen only in the repository maybe consider a prefix like repo_ or dest_ (neither are particularly satisfying but I dont have a better suggestion atm).

thereseanders commented 5 years ago

Oh the fs::path() solution is great. Thank you!

I changed the folder parameter to repo_folder.