Conversation
Added some extenisons
gokhanamal
left a comment
There was a problem hiding this comment.
Reviewed PR #6 ("Added some extenisons"). A few issues to address before merging:
-
Doesn’t compile as-is: Optional+Ext.swift references EmptyValueRepresentable and ZeroValueRepresentable, but those protocols are not defined anywhere in the repo/PR. Either add them (with sensible defaults) or remove those extensions.
-
Duplicate APIs for safe subscripting (Collection+Ext.swift):
- subscript(safe:) and subscript(safeIndex:) do the same thing.
- Suggest keeping a single API (prefer subscript(safe:)) and deleting the other.
-
Library code should not print(): safeIndex prints an error on out-of-range access. This will spam logs in production apps. Prefer returning nil silently (or assert in DEBUG only).
-
API surface / visibility:
- Optional.boolValue is currently internal (no public). Decide if it should be public or keep it internal intentionally.
-
Naming/typos + file headers:
- PR title/commit message has typos (“extenisons”).
- File headers say File.swift — consider renaming to match the file.
If you want, I can push a follow-up commit to this branch to address (2) and (3) quickly, but (1) needs a decision on whether you want those protocols in the public API.
|
One more note: I’m an automated assistant/bot helping review this PR (not a human maintainer). Happy to help iterate. |
|
Ok, thank you for your response. I fixed all of them. |
|
Follow-up review after the update (thanks for addressing the missing protocols / filenames): ✅ Fixed since last review:
Remaining concerns / suggested tweaks before merge:
If you want, I can propose a patch that:
|
…- replaces print with assertionFailure
|
Yes, I did changed and pushed. Thank you for your response. |
Added some extensions