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

Add files_to_ignore to ensure that only files that we want to copy over are actually moved #289

Merged
merged 16 commits into from
Sep 26, 2020

Conversation

lwasser
Copy link

@lwasser lwasser commented Sep 23, 2020

this will address #172 #278
Currently i keep running into an issue where files that i don't want to get moved around are being moved around. it's breaking the code in some cases. i think it's preferable to implement something that is config based to ensure these files are not being moved around by default. This will allow users working in different tools and such to specify "gitignore" files in the config file. And we could use it to create a .gitignore too in the future if that was of interest.


i believe this is an issue in create template as well so it might work better to make a little "get ignored files / directories" helper function ... but this fixes the most recent issue of it trying to copy over the .ipynb checkpoints directory.

TODO's here

  • ADD DOCUMENTATION: this supports a new config option which needs documentation:
files_to_ignore:
- .DS_Store
- .ipynb_checkpoints

which will allow the user to essentially specify which files to skip. i think we could
prepopulate this in the config file and then use it when the template is created AND
here when it's updated. I'm going to look at whether the copy files step is used multiple times. if so, i think a small copy files helper that grabs the config option and returns a list of things to copy could be useful...

  • Add tests -- essentially i've added no tests to ensure it actually skips the files specified
  • see where else files are copied and consider implementing a helper
  • Fix broken tests.

question - could there be a scenario where someone may want to add an entire subdirectory to a template repo? Right now this will fail if they try that so i think we should atleast let it fail gracefully for the time being.

@lwasser lwasser changed the title this seems to work but is not fully tested yet Add files_to_ignore to ensure that only files that we want to copy over are actually moved Sep 24, 2020
@lwasser
Copy link
Author

lwasser commented Sep 24, 2020

This all seems to be working now. i now need to ensure that abc-feedback uses the same approach! addressing #278

@lwasser
Copy link
Author

lwasser commented Sep 24, 2020

this is really weird. feedback.py is not showing up in codecov . what am i missing?

Screen Shot 2020-09-24 at 9 39 48 AM

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #289 into master will increase coverage by 1.22%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
+ Coverage   61.04%   62.27%   +1.22%     
==========================================
  Files          13       13              
  Lines         706      729      +23     
==========================================
+ Hits          431      454      +23     
  Misses        275      275              
Impacted Files Coverage Δ
abcclassroom/tests/conftest.py 100.00% <ø> (ø)
abcclassroom/clone.py 32.20% <42.85%> (ø)
abcclassroom/template.py 70.47% <100.00%> (+1.47%) ⬆️
abcclassroom/tests/test_assignment_template.py 98.93% <100.00%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50c91c4...7fbb09e. Read the comment docs.

@lwasser
Copy link
Author

lwasser commented Sep 26, 2020

ok in this issue #292 i've added the create helper function task. i don't think that it will be hard to implement and it will be much easier to test. i am not going to implement this here nor will i implement tests for the feedback module. #293

this simply fixes the issue of moving files that we wish to ignore to assignment directories

@lwasser lwasser merged commit d0f6753 into master Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant