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

refactor: cleaned up workspaceError enum in biome_service/diagnostics.rs #4681

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

sikehish
Copy link
Contributor

@sikehish sikehish commented Dec 2, 2024

Summary

closes #4668

I have removed the following unused variants from the WorkspaceError enum:

  • ReportNotSerializable
  • DirtyWorkspace
  • CantReadDirectory

The motivation behind these changes is to eliminate unused code, thereby reducing bloat and improving maintainability.

- Removed ReportNotSerializable, DirtyWorkspace and CantReadDirectory as they were unused.
@github-actions github-actions bot added the A-Project Area: project label Dec 2, 2024
@sikehish sikehish changed the title biome_service/diagnostics.rs: Cleaned up WorkspaceError enum refactor: Cleaned up WorkspaceError enum in biome_service/diagnostics.rs Dec 2, 2024
@sikehish sikehish changed the title refactor: Cleaned up WorkspaceError enum in biome_service/diagnostics.rs refactor: cleaned up workspaceError enum in biome_service/diagnostics.rs Dec 2, 2024
Copy link
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

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

Looks good! Less code is always better.

FYI, GitHub only properly links the issue if you put "fixes #x" or "closes #x" in the PR description.

@@ -186,9 +174,6 @@ pub struct DirtyWorkspace;
description = "The report can't be serialized, here's why: {reason}"
)
)]
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to remove the directives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ematipico . Appolgize for it; missed it due to oversight. I've made the change.

@sikehish
Copy link
Contributor Author

sikehish commented Dec 3, 2024

Looks good! Less code is always better.

FYI, GitHub only properly links the issue if you put "fixes #x" or "closes #x" in the PR description.

I've tagged the issue in the PR description. Do I need to reword it?

@ematipico
Copy link
Member

ematipico commented Dec 3, 2024

@sikehish yes, using the words "closes #xxx" or "fixes #xxx`, GitHub will automatically close the issue once the PR is merged

https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue

@sikehish
Copy link
Contributor Author

sikehish commented Dec 3, 2024

@sikehish yes, using the words "closes #xxx" or "fixes #xxx`, GitHub will automatically close the issue once the PR is merged

https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue

Thanks for the information! I've updated the PR message. I've also made the change, let me know if anything else needs to be changed :)

@ematipico
Copy link
Member

The CI is failing. You need to format the project. Use just format

@sikehish
Copy link
Contributor Author

sikehish commented Dec 3, 2024

The CI is failing. You need to format the project. Use just format

Yup, completely forgot to format. Anything else that needs to be done?

@ematipico ematipico merged commit d808057 into biomejs:main Dec 3, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Project Area: project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Clean up WorkspaceError
3 participants