Skip to content

update SA1518 to ignore empty (0 bytes) files #3927

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DWIAltonaAnalytics
Copy link

closes #3926

Copy link

codecov bot commented May 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.33%. Comparing base (f5843ae) to head (f51d23e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3927      +/-   ##
==========================================
- Coverage   97.44%   97.33%   -0.12%     
==========================================
  Files         926      931       +5     
  Lines      110283   110359      +76     
  Branches     3319     3322       +3     
==========================================
- Hits       107467   107413      -54     
- Misses       1847     1968     +121     
- Partials      969      978       +9     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

var root = tree.GetRoot(cancellationToken);
var firstToken = root.GetFirstToken(includeZeroWidth: true);

return firstToken.IsKind(SyntaxKind.EndOfFileToken) && firstToken.FullSpan.Length == 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests for SA1518 can be improved by checking a file with a single space or something like that. firstToken.FullSpan.Length == 0 can currently be removed without any tests failing,

Copy link
Author

Choose a reason for hiding this comment

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

I added two test cases with white space only "files".
Of those (depending on the setting) one would fail if the IsEmpty helper does not check if the token is empty.

@@ -72,6 +72,12 @@ public override void Initialize(AnalysisContext context)

private static void HandleSyntaxTree(SyntaxTreeAnalysisContext context, StyleCopSettings settings)
{
if (context.Tree.IsEmpty(context.CancellationToken))
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering how rare this is, I would have added this check as the last thing before reporting the diagnostic, to save a couple of clock cycles when no diagnostics are reported.

Copy link
Author

Choose a reason for hiding this comment

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

agreed and done.

@bjornhellander
Copy link
Contributor

Looks good to me

@DWIAltonaAnalytics
Copy link
Author

DWIAltonaAnalytics commented Jun 3, 2025

No idea why the tests are hanging.

If someone with the access rights could trigger them again, please?

@DWIAltonaAnalytics
Copy link
Author

I am tempted to push a non-change commit to trigger the pipeline again :)

If any of the owners read this, please restart the pipeline.

@bjornhellander can you approve, or does that require higher level of user rights?

@bjornhellander
Copy link
Contributor

@DWIAltonaAnalytics, no I can't approve pull requests. @sharwell is the one you need.

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.

SA1518 should not trigger on empty files
2 participants