Skip to content

Conversation

@akkaygin
Copy link
Contributor

@akkaygin akkaygin commented Apr 12, 2021

Summary of the Pull Request

Commit dbbe820 seems to change where OpenConsole.psm1 puts clang-format.exe but /doc/building.md still uses the path from commit 9b92986.
Updated clang-format.exe's path and added the required steps to generate a clang-format.exe file.

References

PR Checklist

  • Closes doc/building.md Possibly Outdated #9777
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx (building.md doesn't exist in docs repo)
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

generation of a clang-format.exe when using VisualStudio
@ghost ghost added the Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs label Apr 12, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

This is true, but it's not technically the most correct guidance here. Technically, all you need to do is a nuget restore to get clang-format pulled down onto your machine. Those mentioned commands will work, but only because they're also calling nuget restore 😄

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 12, 2021
@akkaygin
Copy link
Contributor Author

I see, would adding a step where the user only executes dep\nuget\nuget.exe restore tools\packages.config be a better solution? Maybe put that in a function like Get-CodeFormat? That would look like this:

...To download the required clang-format.exe file, follow one of the building instructions below and run

Import-Module .\tools\OpenConsole.psm1
Set-MsBuildDevEnvironment
Get-CodeFormat

...

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 12, 2021
@zadjii-msft
Copy link
Member

I see, would adding a step...

Yea great idea, that all looks good to me!

updated the documentation accordingly.
@akkaygin akkaygin marked this pull request as ready for review April 17, 2021 17:25
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Thanks!

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Apr 21, 2021
Export-ModuleMember -Function Set-MsbuildDevEnvironment,Invoke-OpenConsoleTests,Invoke-OpenConsoleBuild,Start-OpenConsole,Debug-OpenConsole,Invoke-CodeFormat,Invoke-XamlFormat,Verify-XamlFormat
#.SYNOPSIS
# Download clang-format.exe required for code formatting
function Get-Format()
Copy link
Member

Choose a reason for hiding this comment

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

I'm not in love with the exposed member name here, but... this will be fine. Get- isn't totally correct for what this does; it's more of an Install-

@DHowett DHowett changed the title Updated /doc/building.md doc, tools: Improve docs around using clang-format with VS Apr 21, 2021
@DHowett DHowett merged commit dfb48f4 into microsoft:main Apr 21, 2021
@akkaygin akkaygin deleted the doc-fix branch April 21, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Needs-Second It's a PR that needs another sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants