Navigation Menu

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

[Ready for review] Added CDN Profiles to fluent #1257

Closed
wants to merge 11 commits into from
Closed

[Ready for review] Added CDN Profiles to fluent #1257

wants to merge 11 commits into from

Conversation

hovsepm
Copy link
Contributor

@hovsepm hovsepm commented Nov 2, 2016

No description provided.

@hovsepm hovsepm changed the title [In Progress] Added CDN Profiles to fluent [Ready for review] Added CDN Profiles to fluent Nov 17, 2016
Wrapper<EndpointInner> {

/**
* Get the originHostHeader value.

Choose a reason for hiding this comment

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

We use the 3rd person declarative mood for member descriptions, so "Gets" not "Get", "Specifies" not "Specify", etc.
pls scrub the Javadocs to get them consistent with that. thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed everywhere in CDN public interfaces

/**
* Get the originHostHeader value.
*
* @return the originHostHeader value

Choose a reason for hiding this comment

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

this is not a user-facing statement, as it is meaningless. i know this is the wording in the auto-gen'd one. We need to make it sound user-facing. i.e "origin host header" not "originHostHeader" value. pls scrub the javadocs with this in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed everywhere.

/**
* Get the isHttpsAllowed value.
*
* @return the isHttpsAllowed value

Choose a reason for hiding this comment

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

for the Boolean, one descriptive way would be to say "true is HTTP is allowed, else false"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Wrapper<EndpointInner> {

/**
* Get the originHostHeader value.

Choose a reason for hiding this comment

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

We use the 3rd person declarative mood for member descriptions, so "Gets" not "Get", "Specifies" not "Specify", etc.
pls scrub the Javadocs to get them consistent with that. thx

Btw - on parameters properties returning a value, we don't need the description, just the @return, so:
@return origin host header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

*/
interface PremiumEndpoint<ParentT> {
/**
* Specifies the Origin of the CDN Endpoint.

Choose a reason for hiding this comment

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

there is no need to capitalize "origin"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
}

/** The final stage of the CDN profile Standard Akamai or Standard Verizone endpoint definition.

Choose a reason for hiding this comment

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

Verizon misspelled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

WithStandardAttach<ParentT> withCustomDomain(String hostName);
}

/** The final stage of the CDN profile Premium Verizone endpoint definition.

Choose a reason for hiding this comment

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

pls correct all occurrences of the misspelled "Verizone"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

UpdateDefinitionStages.WithStandardAttach<ParentT> withOrigin(String originName, String originHostName);

/**
* Specifies the Origin of the CDN Endpoint.

Choose a reason for hiding this comment

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

no need to capitalize "origin"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* @param profileName Name of the CDN profile which is unique within the resource group.
* @param endpointName Name of the endpoint under the profile which is unique globally.
*/
void endpointStart(String resourceGroupName, String profileName, String endpointName);

Choose a reason for hiding this comment

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

method names that indicate action start with a verb, not noun --> startEndpoint()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* @param profileName Name of the CDN profile which is unique within the resource group.
* @param endpointName Name of the endpoint under the profile which is unique globally.
*/
void endpointStop(String resourceGroupName, String profileName, String endpointName);

Choose a reason for hiding this comment

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

stopEndpoint(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* @param endpointName Name of the endpoint under the profile which is unique globally.
* @param contentPaths The path to the content to be purged. Can describe a file path or a wild card directory.
*/
void endpointPurgeContent(String resourceGroupName, String profileName, String endpointName, List<String> contentPaths);

Choose a reason for hiding this comment

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

--> purgeEndpointContent(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

* @param endpointName Name of the endpoint under the profile which is unique globally.
* @param contentPaths The path to the content to be loaded. Should describe a file path.
*/
void endpointLoadContent(String resourceGroupName, String profileName, String endpointName, List<String> contentPaths);

Choose a reason for hiding this comment

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

--> loadEndpointContent(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,65 @@
/**

Choose a reason for hiding this comment

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

in the future, pls do not include auto-gen'd code with hand-written code in the same PR. Makes full code reviews pretty much impossible :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link

@martinsawicki martinsawicki left a comment

Choose a reason for hiding this comment

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

could we pls have this PR split into PR with auto-gen'd code and PR with hand-written code? Having them all mixed together makes a proper code review pretty much impossible :-)

@hovsepm
Copy link
Contributor Author

hovsepm commented Nov 21, 2016

Working on rebase to split generated and handcrafted files in two separate commits that will make code review easier...

sima-zhu pushed a commit to sima-zhu/azure-sdk-for-java that referenced this pull request Mar 21, 2019
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.

None yet

3 participants