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 rule S6797: Blazor query parameter should be of supported type #8333

Merged
merged 18 commits into from
Nov 16, 2023

Conversation

sebastien-marichal
Copy link
Contributor

Fixes #8274

@sebastien-marichal sebastien-marichal force-pushed the sebastien/S6797-query-param branch from 63d9dfe to 10533ff Compare November 8, 2023 09:49
@sebastien-marichal sebastien-marichal marked this pull request as ready for review November 8, 2023 14:26
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this rule and S6803 both target the same properties and do the same checks I think we should sync to create a baseclass that will do the context validation and return the relevant nodes:

  • Registration
  • Generic check to exclude non razor stuff
  • Checks for Route attribute
  • Checks properties attributes
  • Return properties

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should also be feasible to bump the coverage to 100% 👍

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource force-pushed the cristian/new-rule-S6803 branch 2 times, most recently from 20476b8 to 838932a Compare November 14, 2023 14:14
Base automatically changed from cristian/new-rule-S6803 to master November 14, 2023 16:20
@sebastien-marichal sebastien-marichal force-pushed the sebastien/S6797-query-param branch from 50f1d96 to 18c4ef3 Compare November 15, 2023 13:55
Copy link
Contributor

Choose a reason for hiding this comment

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

We are close!

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM! I left one optional comment, I'll let you choose whether to merge it as is or not

@@ -0,0 +1 @@
@page "/query-parameters"

Choose a reason for hiding this comment

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

This one might have explicit @namespace RazorClassLib.S6797 as well

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

98.0% 98.0% Coverage
0.0% 0.0% Duplication

@sebastien-marichal sebastien-marichal merged commit 9f6edc4 into master Nov 16, 2023
24 checks passed
@sebastien-marichal sebastien-marichal deleted the sebastien/S6797-query-param branch November 16, 2023 16:38
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.

New rule S6797: Query parameter should be of supported type
2 participants