Porting System.Security.Claims#4933
Conversation
|
@weshaggard : Please have a look. |
There was a problem hiding this comment.
Can any of these fields be readonly?
There was a problem hiding this comment.
they are used to hold property values that are just {get}. Today's coding practice is usually { private set }.
There was a problem hiding this comment.
But at the very least a field like _properties should be readonly.
There was a problem hiding this comment.
A user can add a property at any time.
There was a problem hiding this comment.
That doesn't affect whether the dictionary field can be readonly.
|
A couple other questions:
|
There was a problem hiding this comment.
Could _externalClaims be typed as List<List<Claim>> instead of Collection<IEnumerable<Claim>>? It looks like this is only used internally and the IEnumerable<Claim>s are always List<T> already. This would be slightly more efficient than using Collection<T> and would avoid enumerator allocations when enumerating each item in the Claims property as List<T>'s struct enumerator would be used.
There was a problem hiding this comment.
Yes they are only used internally and was designed for holding claims from 'roleProviders'.
I don't see any reason to avoid improving this.
|
Opened a tracking issue for the proposed refactoring #4935 |
venkat-raman251
force-pushed
the
master
branch
from
f497210 to
a19c331
Compare
There was a problem hiding this comment.
Nit: "claim" is sometimes capitalized and sometimes not. Not clear to me the rhyme or reason for it.
There was a problem hiding this comment.
Since it is a type, how about 'Claim'? Capitalized.
There was a problem hiding this comment.
I'm happy either way, as long as we're consistent 😄
|
Are there any tests to go with the library? |
9 participants
No description provided.