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.

Sign up for GitHub

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

Jump to bottom

Fix various C# exceptions #64900

Merged
merged 1 commit into from
Aug 28, 2022
Merged

Fix various C# exceptions #64900

merged 1 commit into from
Aug 28, 2022

Conversation

raulsntos
Copy link
Member

  • Replace IndexOutOfRangeException with ArgumentOutOfRangeException.
  • Replace Exception with a more specific exception.
  • Add the parameter name to argument exception.
  • Update documentation for methods that throw exceptions.
  • Use StringBuilder to build exception messages.
  • Ensure exception messages end with a period.

I couldn't think of a better exception than InvalidOperationException to replace Exception but I'm not sure it's the best option in all cases.

@raulsntos raulsntos added this to the 4.0 milestone Aug 25, 2022
@raulsntos raulsntos requested a review from a team August 25, 2022 18:37
@neikeq
Copy link
Contributor

neikeq commented Aug 26, 2022

Is InvalidOperationException the right exception to throw when a process fails or another reason that's not related to the object state?

The exception that is thrown when a method call is invalid for the object's current state.

Throwing an InvalidOperationException exception

You should throw an InvalidOperationException exception only when the state of your object for some reason does not support a particular method call. That is, the method call is valid in some circumstances or contexts, but is invalid in others.

https://docs.microsoft.com/en-us/dotnet/api/system.invalidoperationexception?view=net-6.0#Throwing

@raulsntos
Copy link
Member Author

Is InvalidOperationException the right exception to throw when a process fails or another reason that's not related to the object state?

I'm not sure but I can't think of a better exception. Even if it's not related to the object's state it's somewhat related to the state of the project as a whole (for example if the build/publish process fails) so maybe it's ok, otherwise we may consider implementing our own exceptions for these cases (see https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2201).

@raulsntos raulsntos force-pushed the dotnet/fix-exceptions branch from b1e8737 to 0610bce Compare August 26, 2022 10:31
@neikeq
Copy link
Contributor

neikeq commented Aug 26, 2022 •
edited
Loading

We could always change those InvalidOperationExceptions in the future. It's not worse than throwing Exception as we're doing currently.

@raulsntos raulsntos force-pushed the dotnet/fix-exceptions branch from 0610bce to f069ae4 Compare August 26, 2022 14:44
@raulsntos
Fix various C# exceptions
79f9f59 
- Replace `IndexOutOfRangeException` with `ArgumentOutOfRangeException`
- Replace `Exception` with a more specific exception
- Add the parameter name to argument exception
- Update documentation for methods that throw exceptions
- Use `StringBuilder` to build exception messages
- Ensure exception messages end with a period
@raulsntos raulsntos force-pushed the dotnet/fix-exceptions branch from f069ae4 to 79f9f59 Compare August 26, 2022 14:56
@neikeq neikeq merged commit 58f8f3a into godotengine:master Aug 28, 2022
@raulsntos raulsntos deleted the dotnet/fix-exceptions branch August 28, 2022 23:28
Anutrix
Anutrix reviewed Aug 29, 2022
View reviewed changes
modules/mono/editor/GodotTools/GodotTools/Build/BuildOutputView.cs
@@ -293,15 +293,15 @@ private void ScrollToLastNonEmptyLogLine()
public void RestartBuild()
{
if (!HasBuildExited)
throw new InvalidOperationException("Build already started");
throw new InvalidOperationException("Build already started.");

BuildManager.RestartBuild(this);
}

public void StopBuild()
{
if (!HasBuildExited)
Copy link
Contributor

@Anutrix Anutrix Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm new to this but shouldn't this be HasBuildExited instead of !HasBuildExited? Won't an in-progress build have this value as false?
I'm just trying to understand it.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, but looking a bit more into this it seems these RestartBuild and StopBuild methods are not used and the BuildManager methods they call throw a NotImplementedException. It seems they were never really used, since the rebuild and stop buttons are implemented in MSPanel (previously named BottomPanel).

Also, building is always a blocking operation so these methods would never be executed if a build is in-progress anyway although we do want to support building in the background (see godotengine/godot-proposals#849 (comment)), it's just not implemented yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarkKilauea DarkKilauea DarkKilauea left review comments

@Anutrix Anutrix Anutrix left review comments

@neikeq neikeq neikeq approved these changes

Assignees
No one assigned
Labels
enhancement topic:dotnet
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@raulsntos @neikeq @DarkKilauea @Anutrix