Fix enumerating zip with deleted entries #53
Conversation
BretJohnson
requested review from
jonpryor,
grendello,
dellis1972 and
jonathanpeppers
BretJohnson
force-pushed
the
skip-deleted-items-when-enumerating
branch
9 times, most recently
from
7f8b3ea to
82617b5
Compare
Previously, if a zip had deleted entries then enumerating the entries in the zip would throw an exception - the ReadEntry call on a deleted entry would thrown an exception. With this change, deleted entries are instead skipped in the enumeration.
BretJohnson
force-pushed
the
skip-deleted-items-when-enumerating
branch
from
82617b5 to
80b0a8f
Compare
ZipArchive.cs
Outdated
| /// <param name="index">index to read</param> | ||
| /// <param name="returnNullIfDeleted">whether to return null or throw an exception for deleted entries</param> | ||
| /// <returns></returns> | ||
| public ZipEntry ReadEntry (ulong index, bool returnNullIfDeleted = false) |
There was a problem hiding this comment.
Should this maybe mirror an example in the BCL? Like Type.GetType:
They named this throwOnError, in this case we can default it to true.
This is also an ABI break to use an optional parameter. Can we leave the old method signature in, and just call the other method? (default pass in true)
There was a problem hiding this comment.
I actually had done that at first, and just changed it back, per your suggestion. So now the parameter is throwIfDeleted (since non delete errors always throw) and I updated the API to maintain backward ABI compatibility.
| /// <param name="index">index to read</param> | ||
| /// <param name="throwIfDeleted">whether to return null or throw an exception for deleted entries</param> | ||
| /// <returns></returns> | ||
| public ZipEntry ReadEntry (ulong index, bool throwIfDeleted) |
There was a problem hiding this comment.
Presumably at some point in the future, we'll want to enable C#8 Nullable Reference Types in this repo.
Should we do so, this method would need to return ZipEntry?.
An alternative approach would be to leave this method unchanged as ZipEntry ReadEntry(long index), and add a new method which returns a ZipEntry?.
Thus, a "new' method instead of a ReadEntry() overload.
Question is, what should said new method be named?
There was a problem hiding this comment.
We leave it with the same name. Overloads don't need to have identical return types.
Previously, if a zip had deleted entries then enumerating the entries in the zip would throw an exception - the ReadEntry call on a deleted entry would thrown an exception. With this change, deleted entries are instead skipped in the enumeration.