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

Renamed files appear within modified_files, but fail when passed to danger.git.diffForFile #481

Open
neil-morrison44 opened this issue Jan 16, 2018 · 8 comments

Comments

@neil-morrison44
Copy link

I've got a file within my repo which has had a parent folder renamed
(e.g. from /tests/seach/index.spec.js to /tests/search/index.spec.js)

I'm running some tests on the diff of modified files, and this renamed (but not modified) file is turning up within danger.git.modified_files.

It appears that when I run danger.git.diffForFile on the file's path, danger fires off 1 request for the branch file (which succeeds) and 1 request for the master file using the new path (which fails, 404).

I'd expect Danger to either:

  • Know that the file has been renamed but not modified, not include it within danger.git.modified_files
  • If that's not possible then include it within danger.git.created_files
  • Handle renames, request the old version using the old name (if possible?)
  • More graceful handling / error reporting of 404s during the requests for diffs
@orta
Copy link
Member

orta commented Jan 21, 2018

I agree, that logic should be pretty feasible to look up using the existing calls we have (e.g. inside the diff we get) then to use that instead of simply looking for the same named file twice

@lawrence-berry
Copy link

I've also ran into the issues described above with renamed folders, and agree that it shouldn't be included in modified_files and including it in created_files would make more sense if it's not possible to recognise these files as having been moved rather than modified.

@heatherbooker
Copy link

On a related note, when I have a new file added and try to run danger local, diffForFile fails as it tries to run 'git show master:"<my_new_file.ext>"'. It says:

Path '<my_new_file.ext>' exists on disk, but not in 'master'.

But if I change it to JSONPatchForFile, it reports the diff with no errors. So I think it might extend to created_files as well as renamed ones.

@heatherbooker
Copy link

I spoke too soon - it reports an empty diff if I use JSONPatchForFile, but at least it doesn't crash. ><

@mAAdhaTTah
Copy link

Experiencing this, although I'm not sure it's a major issue. How can we tell that a file has been renamed?

@airtonix
Copy link

airtonix commented Aug 9, 2022

This hasn't been fixed yet, although #1197 seems to think it has.

@airtonix
Copy link

airtonix commented Aug 9, 2022

for now we're having to preempt failing cases :(

const invalidFiles = (
    await Promise.all(
      changedPackageJsons.map(async (filename) => {
        try {
          await execasync(`git show master:"${filename}"`);
        } catch (error) {
          return {};
        }
        const diff = await danger.git.JSONDiffForFile(filename);
        return {
          filename,
          versionChanged: Object.keys(diff).includes('version'),
        };
      })
    )
  ).filter(({ versionChanged }) => versionChanged);

@Uldraxiel
Copy link

I see that the ruby version of danger has a dedicated renamed_files getter in its git API.
In the js API I find no way to detect a rename and to get the path diff.

What's surprising is that the deleted_files getter prefixes its path with 'a/' --dst-prefix='b/', so I expected to see the same pattern in the renamed files with both path. This alone would already allow to detect a rename and extract the after/before path.

Instead, the renaming is treated like a simple modification and only gives the renamed path.

This is a very confusing behavior.

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

No branches or pull requests

7 participants