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/tasks linting #2176

Merged
merged 17 commits into from
Dec 3, 2024
Merged

chore/tasks linting #2176

merged 17 commits into from
Dec 3, 2024

Conversation

baywet
Copy link
Member

@baywet baywet commented Aug 28, 2024

  • chore: removes extraneous project
  • chore: aligns task usage
Microsoft Reviewers: Open in CodeFlow

@baywet baywet self-assigned this Aug 28, 2024
@baywet baywet requested a review from a team as a code owner August 28, 2024 17:41
@baywet baywet enabled auto-merge August 28, 2024 17:41
@baywet baywet force-pushed the chore/tasks-linting branch from dcb0d98 to 094f517 Compare August 28, 2024 19:18
@baywet
Copy link
Member Author

baywet commented Aug 28, 2024

@irvinesunday @thewahome @millicentachieng Some additional context here:
I wanted to test out the analyzers exploration I'm currently doing for internal APIs, and I thought that devx API would be a good guinea pig.
~80%+ of the changes are adding the analyzers + running dotnet format analyzers to automagically fix the issues.
The remaining 20% that I had to go through manually and were a bit painful:

  • Realigning interfaces with the changed implementations
  • Snippet model had a call to async method + GetResult in the constructor, I removed that and made the caller call the async method. We might need some additional cleanup refactoring in the future, but since the PR is already large, I didn't want to over complicate it.
  • We had strange lock implementations to work around race conditions, I cleaned that up with an async lock.
  • cleaned up a bunch of the OData snippet generation with don't need anymore (in fact, once TS is GA, we'll be able to get rid of all of that)
  • used AsyncLazy where we were doing strange Lazy loading for async resources.

When you review I suggest that:

  • you check the "hide whitespace" thing
  • you can go fast on the unit test changes
  • please pay close attention to the sample and permission store, and try it out locally.

Let me know if you have any additional comments or questions.

@baywet baywet force-pushed the chore/tasks-linting branch from 06e9887 to bc8f6ef Compare September 4, 2024 16:25
@millicentachieng
Copy link
Contributor

@baywet The snippets pipeline is failing due to timeouts. Further investigation is needed to resolve the underlying issue.

@baywet baywet force-pushed the chore/tasks-linting branch from 812ee47 to d104dd2 Compare October 24, 2024 12:36
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

Thanks for the effort here @baywet.

We'll still need to run some validations on this before merging as you suggested. Otherwise, just a few comments from my side.

CodeSnippetsReflection.App/Program.cs Show resolved Hide resolved
CodeSnippetsReflection.App/Program.cs Show resolved Hide resolved
CodeSnippetsReflection/SnippetBaseModel.cs Show resolved Hide resolved
FileService/Interfaces/IFileUtility.cs Show resolved Hide resolved
andrueastman
andrueastman previously approved these changes Nov 19, 2024
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

Disabling auto-merge for now so that @millicentachieng can take a look before we finally merge.

Copy link

sonarqubecloud bot commented Dec 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
68.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@andrueastman andrueastman merged commit 2960f2b into dev Dec 3, 2024
11 of 12 checks passed
@andrueastman andrueastman deleted the chore/tasks-linting branch December 3, 2024 13:24
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.

3 participants