enabled passing of corrupt files to map

[hannahc243]

Fixes #3688 which allows passing of corrupt files to map

[hayesla]

Thanks for the PR @hannahc243 and congrats on your first contribution ! 🥳

[ayshih]

Thanks for the PR! Some immediate thoughts:

• Don't add a "bad" test file to the repository, much less one that is named to look like a real file! Such an approach is bound to cause confusion and breakages, as you've already discovered with the unrelated unit test that you had to separately fix. Instead, define a pytest fixture that creates a temporary "bad" test file for use in the unit tests.
• Naming the keyword silence_errors makes me uneasy because it still emits warnings, so is definitely not silent. I'd probably name the keyword something like skip_errors, but others might have better suggestions.

[ayshih]

Maybe allow errors?

[nabobalis]

• Don't add a "bad" test file to the repository, much less one that is named to look like a real file! Such an approach is bound to cause confusion and breakages, as you've already discovered with the unrelated unit test that you had to separately fix. Instead, define a pytest fixture that creates a temporary "bad" test file for use in the unit tests.

I don't see the harm in a bad test file, I was the one who told them to add it, the filename I think we can work on. It would be nice to have a file that is from a failed download, that we can use easily.

That unrelated unit test I don't think even needs to exist.

• Naming the keyword silence_errors makes me uneasy because it still emits warnings, so is definitely not silent. I'd probably name the keyword something like skip_errors, but others might have better suggestions.

That silence_errors already exists in the Map API, I wanted to avoid adding a new keyword. If you prefer, we can add a new keyword that will handle this behaviour, but since "silence_errors" already does this but for hdu pairs that are passed, extending it to do it for files seems fine to me.

[ayshih]

• Don't add a "bad" test file to the repository, much less one that is named to look like a real file! Such an approach is bound to cause confusion and breakages, as you've already discovered with the unrelated unit test that you had to separately fix. Instead, define a pytest fixture that creates a temporary "bad" test file for use in the unit tests.

I don't see the harm in a bad test file, I was the one who told them to add it, the filename I think we can work on. It would be nice to have a file that is from a failed download, that we can use easily.

I think it'd be of extremely limited value to have in the repository, but I'd probably be fine if the name were simply changed to something like not_actually_a_fits_file.fits.

• Naming the keyword silence_errors makes me uneasy because it still emits warnings, so is definitely not silent. I'd probably name the keyword something like skip_errors, but others might have better suggestions.

That silence_errors already exists in the Map API, I wanted to avoid adding a new keyword. If you prefer, we can add a new keyword that will handle this behaviour, but since "silence_errors" already does this but for hdu pairs that are passed, extending it to do it for files seems fine to me.

Well, then I additionally prefer to change the name of the keyword that already exists in the API. It doesn't make sense that a keyword called silence_errors does not actually silence anything, but instead replaces errors with (non-silent) warnings.

[nabobalis]

Well, then I additionally prefer to change the name of the keyword that already exists in the API. It doesn't make sense that a keyword called silence_errors does not actually silence anything, but instead replaces errors with (non-silent) warnings.

I was hoping you wouldn't say this but I agree.

Would you be ok with this being punted to another PR?

[ayshih]

Yeah, punting is fine

[hayesla]

Thanks for this PR @hannahc243 !

Looks great, just two suggestions

[hayesla]

should we also add a message here to suggest to the user to use the silence_errors keyword to skip over invalid files?

[nabobalis]

I think this is worth adding to my follow up PR renaming the keyword.

[nabobalis]

Thank you for the pull request @hannahc243!