-
Notifications
You must be signed in to change notification settings - Fork 296
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
shortcut for iitem #883
shortcut for iitem #883
Conversation
linq is not available anyway :( |
{ | ||
public static IEnumerator<T> GetEnumerator<T>(this IItems<T> items) | ||
{ | ||
return items.Items.GetEnumerator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, just curious, is it possible for items.Items
to be null in any case? What is the expected behavior? Let it throw or gracefully quit the enumeration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup you are right, would throw npe
maybe make it empty would be better
the issue now is that linq is not compatible with this impl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we are looking for a cheap fix, my 2 cents if you don't mind:
- I still think this is a good change even without linq because of the
foreach
improvement; - Linq could still be used on Items and I don't think it is too terrible:
podList.Items.Where(i => i.abc); // Maybe not as perfect as podList.Where(i => ...) but I think it is okay.
- Of course, that is based on the idea that
interface IItems<T> : IEnumerable<T>
looks like a sizable change, maybe there's an easier way to do it.
On a side note: if we can take a major refactor, why IItems<T>
? Can't it be IEnumerable<T>
to begin with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking into templates to see if can put an IEnumerable
there
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@brendanburns I believe this is ready for merge |
/lgtm Thanks |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, tg123 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
fix #861
thank @WeihanLi for the idea