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

OSOE-760: Disable SA1518 since it's already covered by S113 #96

Merged
merged 1 commit into from
Dec 30, 2023

Conversation

0liver
Copy link
Contributor

@0liver 0liver commented Dec 30, 2023

  • warns on lacking newline at end of file

- warns on lacking newline at end of file
@Piedone
Copy link
Member

Piedone commented Dec 30, 2023

Thanks for the contrib! Don't worry about the missing parent PR failing check, I'll add one.

Any particular reason why you didn't choose to keep the StyleCop rule instead?

@Piedone Piedone changed the title Disable SA1518 since it's already covered by S113 OSOE-760: Disable SA1518 since it's already covered by S113 Dec 30, 2023
@0liver
Copy link
Contributor Author

0liver commented Dec 30, 2023

Thanks! I had already forgotten that a parent PR is needed 😉

As for the choice of rule to keep - I think I liked the warning message better from S113, but looking at the great rules provided by StyleCop, keeping SA1518 instead seems like a good idea, too. Would you like me to switch them around?

@Piedone
Copy link
Member

Piedone commented Dec 30, 2023

No, it's OK then. The StyleCop rule also requires some configuration so that makes things easier. Interestingly enough, we didn't actually add any config but it was still working, perhaps by some coincidence.

@Piedone Piedone merged commit dae7fe5 into Lombiq:dev Dec 30, 2023
5 of 6 checks passed
@Piedone
Copy link
Member

Piedone commented Dec 30, 2023

Thank you!

@0liver
Copy link
Contributor Author

0liver commented Dec 30, 2023

It's been a pleasure! 😊

@0liver
Copy link
Contributor Author

0liver commented Jan 2, 2024

There's another duplicate, CA1711 and S2344:

image

Which one would you remove?

@Piedone
Copy link
Member

Piedone commented Jan 2, 2024

I'd need to dig into that, but the SDK one is usually a safer bet.

@0liver
Copy link
Contributor Author

0liver commented Jan 2, 2024

the SDK one is usually a safer bet.

To keep, I suppose?


I just discovered why picking S113 over SA1518 might have been the wrong choice - there's no quick-fix action available for the former, but there is for the latter, which is greatly appreciated when you want to clean up your solution one warning at a time 😉

Do you fancy another PR to invert the exclusion?

@Piedone
Copy link
Member

Piedone commented Jan 2, 2024

Yes, and sure, thanks.

@0liver
Copy link
Contributor Author

0liver commented Jan 2, 2024

Here's the follow-up PR: #97.

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