Skip to content

Conversation

@naveen521kk
Copy link
Member

List of Changes

  • Add condition for file not found

Motivation

Acknowledgement

@naveen521kk
Copy link
Member Author

@Aathish04 Can you check whether this work by commiting the black-pr.yml in #315 ?

@naveen521kk naveen521kk changed the title Fix File Not Found Error Fix File Not Found Error - Github Actions Sep 5, 2020
@naveen521kk naveen521kk added pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! infrastructure Anything related to our infrastructure labels Sep 5, 2020
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

I think this would not check files that were moved by their new name. There is a simple solution for that:

Currently, you check the diff backwards, i.e.,

 git diff --name-only HEAD HEAD~$($commits.count) > /d/changes.txt

Changing this to

 git diff --name-only HEAD~$($commits.count) HEAD > /d/changes.txt

prints the new file names.

Regarding checking existence of files: I'd rather this were done with git diff itself as well: by querying git diff --diff-filter=d --name-only HEAD~$($commits.count) HEAD, the lowercase d tells diff to ignore deleted files.

@naveen521kk
Copy link
Member Author

naveen521kk commented Sep 5, 2020

I think this would not check files that were moved by their new name.

I don't understand what this mean, It would be good if you provide an example situation. @behackl

Regarding checking existence of files: I'd rather this were done with git diff itself as well: by querying git diff --diff-filter=d --name-only HEAD~$($commits.count) HEAD, the lowercase d tells diff to ignore deleted files.

I think it would be good. But I think the solution I recommended is lot better and easy. But anyhow @behackl If you confirm that it will work I will add it.

@behackl
Copy link
Member

behackl commented Sep 5, 2020

@naveen521kk see this example here:

[behackl@gc7 example]$ git init .
Initialized empty Git repository in /home/behackl/git/example/.git/
[behackl@gc7 example]$ touch some_file
[behackl@gc7 example]$ git add some_file && git commit -m "add a file"
[master (root-commit) acda2b1] add a file
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 some_file
[behackl@gc7 example]$ git checkout -b new_branch
Switched to a new branch 'new_branch'
[behackl@gc7 example]$ git mv some_file another_file
[behackl@gc7 example]$ git commit -m "renamed file"
[new_branch 20e172b] renamed file
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename some_file => another_file (100%)
[behackl@gc7 example]$ git diff --name-only master HEAD
another_file
[behackl@gc7 example]$ git diff --name-only HEAD master
some_file

I create a new repository, add a file some_file, then switch branch and rename the file to another_file. From this new branch, calling git diff --name-only master HEAD (i.e., tracking changes from master to the current commit) yields the file after renaming.

Asking for the diff in the other direction, namely git diff --name-only HEAD master, that is from the current commit to the (old) master, the file name before renaming is printed (and the renamed file would not be in your list).

And regarding your second question: continuing the example and deleting my file:

[behackl@gc7 example]$ git rm another_file 
rm 'another_file'
[behackl@gc7 example]$ git commit -m "removed file"
[new_branch 37b3165] removed file
 1 file changed, 0 insertions(+), 0 deletions(-)
 delete mode 100644 another_file
[behackl@gc7 example]$ git diff --name-only master HEAD
some_file
[behackl@gc7 example]$ git diff --diff-filter=d --name-only master HEAD

As you can see, --diff-filter=d makes it not print the deleted file.

I think it is better to have all these kind of preprocessing steps in one place / line, rather than distributed over two. (But I admit, in this case this is only a very weak preference.)

@naveen521kk
Copy link
Member Author

Ok then I will make your suggestion in latest commit. What I will do is change

 git diff --name-only HEAD HEAD~$($commits.count) 

to

 git diff --name-only HEAD~$($commits.count) HEAD

@naveen521kk naveen521kk requested a review from behackl September 5, 2020 08:48
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

LGTM.

@naveen521kk
Copy link
Member Author

Ok then this can be merged if any other bugs faced pls ping me.

while read file
do
if [ $(python -c "print('${file}'[-2:])") == "py" ]
if [[ ($(python -c "print('${file}'[-3:])") == ".py") && (-f ${file}) ]]
Copy link
Member

Choose a reason for hiding this comment

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

I have some reservations about using Python instead of Bash for this conditional, but its something we can come back and fix.

This works for the time being, and we have multiple PRs waiting for this one, so approved!

@huguesdevimeux huguesdevimeux merged commit 9824463 into ManimCommunity:master Sep 6, 2020
@naveen521kk naveen521kk deleted the patch-2 branch September 6, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Anything related to our infrastructure pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Black Check Fails as it looks for Deleted/Moved Files

4 participants