Show security heading only if warnings exist

[MarkEWaite]

Only show security heading if there is a security warning

Test highlights that there is ambiguity in the tested code because there are two booleans to represent a single condition. It is possible to have hideWarning == true and showWarning == true, even though we can only do one or the other but not both.

Another pull request or another change in this pull request will need to resolve that issue in the most recently merged pull request.

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[MarkEWaite]

@jiakuanghe are you interested in making the change to replace the two boolean variables that control the visibility of security warnings with a single boolean variable?

If not, I'll make that change.

I didn't consider the case where a programmer (like me writing the tests in this pull request) might make the mistake of assigning conflicting values to those two boolean variables that represent the same condition. The conflict won't be seen by users, but it will make it more difficult to change this code if two variables are used to represent the single condition. I missed that when I reviewed

• Show security warnings by default (#258) #522

[jiakuanghe]

@jiakuanghe are you interested in making the change to replace the two boolean variables that control the visibility of security warnings with a single boolean variable?

If not, I'll make that change.

I didn't consider the case where a programmer (like me writing the tests in this pull request) might make the mistake of assigning conflicting values to those two boolean variables that represent the same condition. The conflict won't be seen by users, but it will make it more difficult to change this code if two variables are used to represent the single condition. I missed that when I reviewed

• Show security warnings by default (#258) #522

@MarkEWaite Yes, I am interested in making the change.

To keep the compatibility with the previous version, I reserved the --view-security-warnings and add --hide-security-warnings. I assumed that --view-security-warnings can be assigned to true or false by users explicitly, but actually it cannot! I am going to remove --view-security-warnings. Thanks for letting me know that!

[MarkEWaite]

I assumed that --view-security-warnings can be assigned to true or false by users explicitly, but actually it cannot! I am going to remove --view-security-warnings. Thanks for letting me know that!

I don't think that we want to remove the --view-security-warnings command line argument. We don't want to break existing users if they were using the command line argument to see security warnings. Rather, the issue is purely that there are two internal variables to represent a single condition. I believe that the old internal variable needs to be deleted and the existing getter needs to read the negated value of the new variable.

[jiakuanghe]

Rather, the issue is purely that there are two internal variables to represent a single condition.

Got it! But what if the user uses the --view-security-warnings and --hide-security-warnings at the same time as the command line argument, we should show the warnings or not?

[jiakuanghe]

I believe that the old internal variable needs to be deleted and the existing getter needs to read the negated value of the new variable.

I agree with you. Just keeping one variable there is better. Can I remove the internal variable showWarnings directly? In this case, if the user use --view-security-warnings command line argument will not have any effects.

[MarkEWaite]

Can I remove the internal variable showWarnings directly? In this case, if the user use --view-security-warnings command line argument will not have any effects.

I think so, though you'll need to check the places that are referring to that variable so that they instead refer to the hideWarnings variable instead (using a negation to invert the value of the hideWarnings variable).

[jiakuanghe]

I think so, though you'll need to check the places that are referring to that variable so that they instead refer to the hideWarnings variable instead (using a negation to invert the value of the hideWarnings variable).

Thanks. I will do that. Do I need to create an issue and quoted it in the PR? Or it is okay to quote this #537 in my new PR?

[MarkEWaite]

Thanks. I will do that. Do I need to create an issue and quoted it in the PR? Or it is okay to quote this #537 in my new PR?

Good enough to mention this in the pull request. If the mention in includes the URL of the comment, I believe GitHub will add a line in the PR history and in the comment that will show they are related to each other.

[jiakuanghe]

@MarkEWaite Done #549! Please let me know if there is anything that needs to be further improved. Thanks!

[freyam]

I have reviewed the PR @jiakuanghe. Great work 🚀