Skip to content
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

New AdvSec & GHAS - Enable using config file #2775

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

shaopeng-gh
Copy link
Collaborator

@shaopeng-gh shaopeng-gh commented Feb 7, 2024

@EasyRhinoMSFT
This is to enable using config file to change settings for New AdvSec & GHAS work.

{
[Serializable]
[JsonConverter(typeof(TypedPropertiesDictionaryConverter))]
public class RuleKindSet : HashSet<RuleKind>
Copy link
Collaborator

@EasyRhinoMSFT EasyRhinoMSFT Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RuleKindSet

Why couldn't we use a HashSet? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is following the same pattern as other fields we have, the ResultKind.
The commanline nuget package the options have some limitation so there is some conversions needed

@@ -319,7 +319,7 @@ public virtual TContext InitializeGlobalContextFromOptions(TOptions options, ref
context.TargetFileSpecifiers = options.TargetFileSpecifiers?.Any() == true ? InitializeStringSet(options.TargetFileSpecifiers) : context.TargetFileSpecifiers;
context.InvocationPropertiesToLog = options.InvocationPropertiesToLog?.Any() == true ? InitializeStringSet(options.InvocationPropertiesToLog) : context.InvocationPropertiesToLog;
context.Traces = options.Trace.Any() ? InitializeStringSet(options.Trace) : context.Traces;
context.RuleKinds = options.RuleKinds;
context.RuleKinds = options.RuleKindOption != null ? options.RuleKinds : context.RuleKinds;
Copy link
Collaborator

@EasyRhinoMSFT EasyRhinoMSFT Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options.RuleKindOption != null ? options.RuleKinds : context.RuleKinds

Can we use this simpler syntax?

options.RuleKindOption ?? context.RuleKinds #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no we can not,
options.RuleKindOption is not the same type as options.RuleKinds

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you see lines above
context.ResultKinds = options.Kind != null ? options.ResultKinds : context.ResultKinds;
Some tricky here, the options.Kind is not the same type as options.ResultKinds.

p.s. I think the naming before is not good for Kind for ResultKinds , it is better to name using xxxOption, or some pattern. People will not know Kind is the command line option for ResultKinds

Copy link
Collaborator

@EasyRhinoMSFT EasyRhinoMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@EasyRhinoMSFT EasyRhinoMSFT merged commit ea17e3c into main Feb 7, 2024
8 checks passed
@EasyRhinoMSFT EasyRhinoMSFT deleted the users/shaopeng-gh/fixconfig branch February 7, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants