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

[TA and Translatior] Update snippets #20552

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Conversation

maririos
Copy link
Member

@heaths
Copy link
Member

heaths commented Apr 21, 2021

Tip: if your commit description says something like "Resolves #20431" or "Fixes #20431", when the PR (when using the commit title/description) is merged the bug is automatically closed.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Given you assign sample strings like string foo = "<replace this>"; which compiles, but then use Uri bar = <replace this> which won't compile, I recommend using the same pattern. If you make sure the code compiles - even with obvious replacement text - you won't even have to use the SNIPPET preproc condition in most of your samples given some clearly isn't executed since the Uri instances you create aren't valid URIs.

@@ -59,26 +62,30 @@ public void BadRequestSnippet()
[Ignore("Samples not working yet")]
public void DocumentTranslationInput()
{
#region Snippet:DocumentTranslationSingleInput
#if SNIPPET
Uri sourceSasUri = <source SAS URI>;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: why not use the same format for the snippet as the actual executed code? At least it will compile, and you wouldn't even need the preproc condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I was leaving it as is so when we turn on the samples we will pass real values. But not sure when we will do this, so I changed it based on your suggestions.
Thanks!

@maririos
Copy link
Member Author

Tip: if your commit description says something like "Resolves #20431" or "Fixes #20431", when the PR (when using the commit title/description) is merged the bug is automatically closed.

Thank you!
I didn't want to do that because I linked to the whole issue that involves more client libraries instead of creating a specific one for TA and Translator (lazy on my side).

@heaths
Copy link
Member

heaths commented Apr 21, 2021

that involves more client libraries

D'oh! You're right. I completely forgot about that (and did the same as you last week). 🤦‍♂️

@maririos maririos merged commit 402c776 into Azure:master Apr 21, 2021
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-net that referenced this pull request Sep 19, 2022
ComputeRP 2022-08-01 release (Azure#20552)

* add files to new version folder

* update version references and readme tags

* update common types version line and reference to common.json

* add fetures for 2022-08-01 release

* remove admin password for ModelValidation CI

Co-authored-by: Theodore Chang <theodore.l.chang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants