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

Added WorkingDirectory property for #7471 #7472

Merged
merged 6 commits into from
May 31, 2024

Conversation

egvijayanand
Copy link
Contributor

Problem

#7471

Solution

Added a property that returns the current directory (have named it WorkingDirectory) from where the dotnet command is invoked.

Checks:

  • Added unit tests

@egvijayanand egvijayanand requested a review from a team as a code owner January 18, 2024 01:31
@baronfel
Copy link
Member

Thank you! This is a great start, there are a few things that need to happen to get this green:

  • the error from the CI systems is complaining about a missing signature in the PublicAPI files - this can be done via an analyzer in VS/VSCode
  • Can you update the docs around the host-provided values here as well?

@egvijayanand
Copy link
Contributor Author

@baronfel I will check those feedback.

@egvijayanand
Copy link
Contributor Author

@baronfel Have updated the advised details, can you review again and let me if anything more is to be done.

@baronfel
Copy link
Member

This looks pretty good to me! There aren't tests for the existing built-ins, so as soon as you can fix the build error around the API surface area this should be good to merge :)

@egvijayanand
Copy link
Contributor Author

This looks pretty good to me! There aren't tests for the existing built-ins, so as soon as you can fix the build error around the API surface area this should be good to merge :)

This is the error message:

src\Microsoft.TemplateEngine.Edge\PublicAPI.Unshipped.txt(11,1): error RS0025: (NETCORE_ENGINEERING_TELEMETRY=Build) The symbol 'Microsoft.TemplateEngine.Edge.DefaultTemplateEngineHost.WorkingDirectory.get -> string!' appears more than once in the public API files (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)

Should I remove these API details from the Unshipped file?

@baronfel
Copy link
Member

It looks like the same line is present multiple times - so just removing the duplicates should be all that you need to do.

@egvijayanand
Copy link
Contributor Author

It looks like the same line is present multiple times - so just removing the duplicates should be all that you need to do.

It's not duplicated and only 11 items are present over there. That's why asking.

image

@egvijayanand
Copy link
Contributor Author

After removing the entry from the Unshipped file, all checks have passed. Can you proceed with the merge?

@egvijayanand
Copy link
Contributor Author

@baronfel can you look into this PR and let me know if anything else needs to be done?

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

This LGTM - thanks for contributing.

@MiYanni / @joeloff - could one of you take a look from the engineering side?

@egvijayanand
Copy link
Contributor Author

Hello @baronfel,

Can the PR be merged as the changes are approved?

@baronfel baronfel merged commit d33e349 into dotnet:main May 31, 2024
10 checks passed
@egvijayanand
Copy link
Contributor Author

Thanks :-)

@egvijayanand
Copy link
Contributor Author

@baronfel, From which .NET SDK version, this property will be made available?

@baronfel
Copy link
Member

baronfel commented Aug 8, 2024

Either .NET SDK 9.0.100-preview.6 (last month's preview) or 9.0.100-preview.7 (this month's preview)

@egvijayanand
Copy link
Contributor Author

Either .NET SDK 9.0.100-preview.6 (last month's preview) or 9.0.100-preview.7 (this month's preview)

Ok thanks, will check.

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