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

chore(build): update analyzers #29

Merged
merged 1 commit into from
Apr 8, 2021
Merged

chore(build): update analyzers #29

merged 1 commit into from
Apr 8, 2021

Conversation

h1dden-da3m0n
Copy link
Contributor

@h1dden-da3m0n h1dden-da3m0n commented Apr 6, 2021

Description

This aims to replace the FxCopAnalyzer with the .Net bundled Analyzer

Changes

  • updates analysers to use .NET Analyzers

Issues

edit:
There are a couple of white space diffs in this, sorry about this! With the introduction/update of the .editorconfig this was to be expected :S

edit2:
I had to add a couple of rule ignores to that I am not 100% sure on and would really appreciate a comment on before merging this.

Closes #24

@h1dden-da3m0n h1dden-da3m0n added the chore This PR contains repository maintenance changes label Apr 6, 2021
Copy link
Member

@oddstr13 oddstr13 left a comment

Choose a reason for hiding this comment

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

Don't worry about the whitespace changes, GitHub can be told to ignore those (gear symbol on the diff view)

@@ -53,7 +63,11 @@
<Rule Id="CA1822" Action="Info" />
<!-- disable warning CA2000: Dispose objects before losing scope -->
<Rule Id="CA2000" Action="Info" />
<!-- disable warning CA5394: Do not use insecure randomness -->
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't look like something we'd want to blanket disable? Should probably be done on a case-by-case basis.

I'll defer this decision to someone with a bit more knowledge about the linting rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I agree, however I used the jellyfin.ruleset from the server repo to update this one. That does not mean that it is flawless or that we should blanket use it everywhere.
However, I know to little about the linting rules myself to make the judgment so it ended up here 🤔

Copy link
Member

Choose a reason for hiding this comment

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

alright, if it came directly from server it's probably fine

Jellyfin.Plugin.Tvdb/Providers/TvdbSeriesProvider.cs Outdated Show resolved Hide resolved
@crobibero crobibero merged commit 8575089 into jellyfin:master Apr 8, 2021
@h1dden-da3m0n h1dden-da3m0n deleted the chore/update-analyzers branch April 8, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore This PR contains repository maintenance changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants