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

FindGuid helper function #3762

Merged
merged 7 commits into from
Dec 2, 2019
Merged

Conversation

mkitzan
Copy link
Contributor

@mkitzan mkitzan commented Nov 28, 2019

Summary of the Pull Request

Adds a simple helper function to look up the GUID associated with a profile name.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Very simple function, for-each through profiles checking for a match. Returns the associated GUID if found, else returns the null GUID.

This function is marked as noexcept to comply with assumption made by other CascadiaSettings functions that Profiles::GetGuid does not throw, despite it throwing.

Validation Steps Performed

The new function is simple and can be visually validated, but added tests regardless. A new test group was added in SettingsTests called TestHelperFunctions to validate the new function FindGuid and older function FindProfile. This test group could be used to validate more helper functions in CascadiaSettings as they're added. The new test group passes after running te.exe TerminalApp.LocalTests.dll.

@mkitzan
Copy link
Contributor Author

mkitzan commented Nov 28, 2019

Does anyone know why the code formatting is failing? The Azure pipeline provides no feedback.

@beviu
Copy link
Contributor

beviu commented Nov 28, 2019

Look at this for code format : https://github.com/microsoft/terminal/blob/master/doc/building.md
There is a runformat command.

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.

Overall looks good, but let's make it a std::optional<GUID>

}
}

return profileGuid;
Copy link
Member

Choose a reason for hiding this comment

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

I think my only real comment is that I think I'd prefer that we make this function return a std::optional<GUID>, and return std::nullopt when we didn't find a match instead of returning the null GUID, since the null GUID is technically a valid GUID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 28, 2019
@mkitzan mkitzan marked this pull request as ready for review November 28, 2019 18:26
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Love this. Thank you!

@zadjii-msft zadjii-msft merged commit e9a3fe5 into microsoft:master Dec 2, 2019
@zadjii-msft
Copy link
Member

Thanks for the contribution!

@zadjii-msft zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Terminal The new Windows Terminal. labels Dec 2, 2019
@mkitzan mkitzan deleted the find-guid-helper branch December 2, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any time we take a profile GUID, consider taking a profile name
4 participants