Adding public API test coverage for Aspire.Hosting.Python #5110
Conversation
…methods#aspire-hosting-python
dotnet-issue-labeler
bot
added
the
area-codeflow
dotnet-policy-service
bot
added
the
community-contribution
…methods#aspire-hosting-python
…ting-python' of github.com:Zombach/aspire into zombach/validate-arguments-of-public-methods#aspire-hosting-python
…methods#aspire-hosting-python
| var action = () => new PythonProjectResource(name, executablePath, projectDirectory); | ||
|
|
||
| var exception = Assert.Throws<ArgumentNullException>(action); | ||
| Assert.Equal("workingDirectory", exception.ParamName); |
There was a problem hiding this comment.
PythonProjectResource's parameter is named projectDirectory which is then passed to the ExecutableResource..ctor to the parameter named workingDirectory.
And because the exception is being thrown from ExecutableResource we get workingDirectory instead of projectDirectory. That sounds confusing for the user. Maybe we should have the null check on the .ctor arguments for PythonProjectResource directly.
@mitchdenny @eerhardt @sebastienros Is there a pattern that we follow in such cases?
There was a problem hiding this comment.
To be honest, I would add a check, it would be more transparent.
But due to the fact that in PythonProjectResource these arguments are not used at the moment. according to CA1062, we can skip this check, since it is performed in depth.
But if someone adds interaction with arguments to PythonProjectResource, then they will have to add a check for 'null'.
That's why I'm in favor of adding it right away.
There was a problem hiding this comment.
https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/using-standard-exception-types
✔️ DO set the ParamName property when throwing one of the subclasses of ArgumentException.
This property represents the name of the parameter that caused the exception to be thrown.
I read this as the ParamName should be set to the name of the parameter that was invalid. In this case projectDirectory.
There was a problem hiding this comment.
I think we need to do the null check in the PythonResource ctor just for clarity. I'm not a fan of the ThrowIfNull method, seems a bit odd to do it that way.
There was a problem hiding this comment.
On second thought it probably doesn't matter to the end result.
…methods#aspire-hosting-python
Zombach
changed the title
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
Fix PR by feedback
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
radical
added
area-app-model
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…methods#aspire-hosting-python
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…methods#aspire-hosting-python
…methods#aspire-hosting-python
|
@Zombach thanks once again for all your work on some of these PRs. Appreciate your efforts. |
Zombach
deleted the
zombach/validate-arguments-of-public-methods#aspire-hosting-python
branch
Initial issues #2649
Issues Check List: Validate arguments of public methods
Microsoft Reviewers: Open in CodeFlow