Skip to content

[Ready for review] Added CDN Profiles to fluent (Rebased)#1288

Merged
martinsawicki merged 2 commits into
Azure:masterfrom
hovsepm:CDNRebase
Nov 23, 2016
Merged

[Ready for review] Added CDN Profiles to fluent (Rebased)#1288
martinsawicki merged 2 commits into
Azure:masterfrom
hovsepm:CDNRebase

Conversation

@hovsepm

@hovsepm hovsepm commented Nov 21, 2016

Copy link
Copy Markdown
Contributor

Rebased PR #1257 into 2 commits. First one is generated code the second one is Hand crafted.

@hovsepm

hovsepm commented Nov 22, 2016

Copy link
Copy Markdown
Contributor Author

@martinsawicki you can review code by commits now. Please let me know if this is still not reviewable.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minor, but for simple "getters" we do not need a description -- @return host header is enough

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

doesn't this return a modifiable list? Might need to go through Collections.unmodifiableList(...). In general, unless they are PagedLists underneath, we return static lists as immutable, which is what unmodifiableList achieves..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed to unmodifiable list.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is this list ultimately returned to the user code? If so, shouldn't it go through Collections.unmodifiableList?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed this as well.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

do we have a test of a "without"-er somewhere as well? (isn't .withoutEndpoint(name) possible?)

@hovsepm hovsepm Nov 22, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@martinsawicki martinsawicki merged commit e246fc1 into Azure:master Nov 23, 2016
@hovsepm hovsepm deleted the CDNRebase branch November 23, 2016 19:42
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