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? |
There was a problem hiding this comment.
This is a duplication of facade\System.Secrit.Claims.csproj configuration. As part of your change you should eliminate the facade folder completely given you have folded it into this project. Also condition this on TargetsWindows=true
There was a problem hiding this comment.
@venkat-raman251 please collapse these net46 configurations.
|
@dotnet-bot Test Innerloop Ubuntu Debug Build and Test please (Jenkins failure) |
There was a problem hiding this comment.
Just curious: why is the override necessary?
venkat-raman251
force-pushed
the
master
branch
from
a19c331 to
3536406
Compare
|
@weshaggard : Updated with all the changes. Also split into 2 commits to identify changes. |
|
LGTM |
Porting System.Security.Claims
9 participants
No description provided.