repobee / repobee-sanitizer

A plugin for sanitizing master repositories before distribution to students
MIT License
2 stars 3 forks source link

[feat] Add pull request workflow #137

Closed tohanss closed 3 years ago

tohanss commented 3 years ago

A first draft

Fix #121

@slarse we think we have the pr branch part working, however, the test to check that the target_branch is not modified seems to fail, the added file that we look for goes missing, perhaps we did something wrong, or just how we read the file in the test. Will keep looking at it but if you find something wrong whenever you have time to look at this that would be great

tohanss commented 3 years ago

The specific error we get is:

 FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-wagmode/pytest-149/test_target_branch_not_modifie0/some-other-file.txt'

on the line:

 other_file_contents_after = other_file.read_text(encoding="utf8")
codecov[bot] commented 3 years ago

Codecov Report

Merging #137 (7c76cfc) into master (9b5e599) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 7c76cfc differs from pull request most recent head 021bae5. Consider uploading reports for the commit 021bae5 to get more accurate results Impacted file tree graph

@@            Coverage Diff            @@
##            master      #137   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         9    +1     
  Lines          255       282   +27     
  Branches        43        49    +6     
=========================================
+ Hits           255       282   +27     
Impacted Files Coverage Δ
repobee_sanitizer/_gitutils.py 100.00% <100.00%> (ø)
repobee_sanitizer/_sanitize_repo.py 100.00% <100.00%> (ø)
repobee_sanitizer/sanitizer.py 100.00% <100.00%> (ø)
repobee_sanitizer/_syntax.py 100.00% <0.00%> (ø)
repobee_sanitizer/_sanitize.py 100.00% <0.00%> (ø)

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 9b5e599...021bae5. Read the comment docs.

tohanss commented 3 years ago

@slarse Ready for review

tohanss commented 3 years ago

Great feedback, just what i wanted :)

tohanss commented 3 years ago

@slarse Code and tests are finished, I made some comments you can look at, I am also going to go through the documentation and update that

tohanss commented 3 years ago

I also realized we should probably give the user the name of the PR branch so that they can checkout to it and push it

tohanss commented 3 years ago

@slarse I just realised the PR flag can now be used without the targetbranch option, can we make targetbranch required by the CLI or do we manually check that in our code?

tohanss commented 3 years ago

Also, i knew this was going to become a bit whacky because theres a chance that the tests create the timestamp for thr PR faster than the code executes, and that just happened, i will find a way to optimise that a little bit

The time stamp right now is the time in seconds since the "epoch" aka when your device started counting time from. I think seconds is what we want since someone can create multiple PRs within the same minute

slarse commented 3 years ago

The time stamp right now is the time in seconds since the "epoch" aka when your device started counting time from. I think seconds is what we want since someone can create multiple PRs within the same minute

I would make it a human-readable timestamp. Something like str(datetime.datetime.now()).replace(" ", "_").

Is this ready for a looksie or are you still making changes?

tohanss commented 3 years ago

@slarse Yes! I will change the timestamp real quick right now first, please look at my comments along with the review. The only side note that I haven't commented about is possible to push the PR branch along with sanitizing to it (which is probably quite helpful)

we can change the time to this but replace space with underscore

datetime.now.strftime("%Y/%m/%d %H:%M:%S")
2021/07/15 10:28:56
tohanss commented 3 years ago

@slarse Checking input first fixed so many nuisances I had with the code, the main command function is so much cleaner now and I was able to structure everything the way i wanted it

The only remaining thing now that I'm not sure about is if we should push the pr branch for the user or tell them to checkout to the branch manually (which we do now)