TODO: need to add test case for list_archive_files

unit test done.
@mikem @tkopecek @jcupova could you help review this?

@julian8628 +1

If strict is used, previous line will throw GenericError, so this seems to be unreachable block.

If there are some files, but filename doesn't match, it will not end here (nevertheless, return default None at the end of method). I would kill this whole subblock (else/else) and just return None as it is in original code.

If strict is used, previous line will throw GenericError, so this seems to be unreachable block.

If there're one or more than one item returned from files = list_archive_files(archive_id, strict=strict), but no one matched the filename the GenericError will be thrown here, like the test case test_non_existing_file()

If there are some files, but filename doesn't match, it will not end here (nevertheless, return default None at the end of method). I would kill this whole subblock (else/else) and just return None as it is in original code.

Ah yes, the last else block is not necessary. Thanks to point it out.

Sorry, I wasn't clear in original comment. What I'm suggesting is simplification to (no change of logic):

    for file_info in files:
        if file_info['name'] == filename:
            return file_info
    if strict:
        raise koji.GenericError('No such file: %s in archive#%s' % (filename, archive_id))
    return None

Sorry, I wasn't clear in original comment. What I'm suggesting is simplification to (no change of logic):

Updated :wine_glass:

Can you file an issue for the TODO that you added?

In the final else: case in list_archive_files() we are applying a filter to a list that we know to be empty. Perhaps this can be streamlined. Or am I missing something?

nitpick: why escape a single quote when easily avoid it by using double quotes?

long line and grammar a touch awkward, maybe:

If strict is True, raise GenericError if:
  - there are no files found for the archive
  - the archive is not a type we are able to expand

Might also be worth adding this note too:

Regardless of strict, an error will be raised if the archive_id is invalid

So this is going beyond issue #719, which only asks for getArchiveFile to raise errors when the requested filename does not exist in the archive. I'm not convinced that the strict option is really needed in list_archive_files().

On one hand, it's just optional, but... I mean, technically if we are asked to list the files in an empty archive, then [] is the correct answer and probably shouldn't be an error.

In the final else: case in list_archive_files() we are applying a filter to a list that we know to be empty. Perhaps this can be streamlined. Or am I missing something?

how about https://pagure.io/fork/julian8628/koji/c/a990887a6ebcbbb95d97842f6527133f3f589ce8?branch=issue%2F719-2

So this is going beyond issue #719, which only asks for getArchiveFile to raise errors when the requested filename does not exist in the archive. I'm not convinced that the strict option is really needed in list_archive_files().
On one hand, it's just optional, but... I mean, technically if we are asked to list the files in an empty archive, then [] is the correct answer and probably shouldn't be an error.

I made list_archive_files() more strict to throw an error when it's an empty archive, since when calling getArchiveFile(archive_id, filename, strict=True) the error message for empty archive and for non-existing filename in archive filelist should be different. Putting the empty result checking in list_archive_files() seems more straight, and it goes with the same way as issue #721. These two issues are the first time to bring strict argument to list* APIs. I guess way could think them again about if it's meaningful to throw errors when result is an empty list.

My original idea is, to avoid change the default behavior and to make it unnecessary to let client check the result again, this option might be helpful.

In the final else: case in list_archive_files() we are applying a filter to a list that we know to be empty. Perhaps this can be streamlined. Or am I missing something?

how about https://pagure.io/fork/julian8628/koji/c/a990887a6ebcbbb95d97842f6527133f3f589ce8?branch=issue%2F719-2

Yes, that looks better. Except, I don't think we should error on the empty archive case.

Sure, with strict, we can error on btypes or archivetypes that we can't handle. However, if we get to the end and we have an actual filelist from an actual archive that happens to be empty, then this is not an error condition, even with strict on.

I made list_archive_files() more strict to throw an error when it's an empty archive, since when calling getArchiveFile(archive_id, filename, strict=True) the error message for empty archive and for non-existing filename in archive filelist should be different.

There should be no error message for empty archives at all. It is simply not an error. It is correct information, and we have no reason to assume that the archive shouldn't be empty.

Putting the empty result checking in list_archive_files() seems more straight, and it goes with the same way as issue #721. These two issues are the first time to bring strict argument to list* APIs. I guess way could think them again about if it's meaningful to throw errors when result is an empty list.

For calls like get_build, where the input is expected to determine a single valid build, the strict option is a way for the caller to ensure they have a sane value. That doesn't really apply for empty archives here.

My original idea is, to avoid change the default behavior and to make it unnecessary to let client check the result again, this option might be helpful.

Let's put it in a different light. The strict options are generally a convenience for the caller, so that they don't have to put a separate assertion in their own code. They could always just implement strict themselves.

So, in what cases would a caller want to raise an error simply because an archive has no files?

In general, I think the change to get_archive_file is fine, but that the value of the rest is less clear and out of scope of the issue. I'm tempted to say split this up.

Sure, with strict, we can error on btypes or archivetypes that we can't handle. However, if we get to the end and we have an actual filelist from an actual archive that happens to be empty, then this is not an error condition, even with strict on.

Yes, it makes sense. Updated.
Should I move the changes in list_archive_files to a new PR?

Should I move the changes in list_archive_files to a new PR?

I think it's ok now. Going to merge with minor docstring tweak

Commit c344d854 fixes this pull-request