Skip to content

implement clearcache command#13

Closed
xavierdecoster wants to merge 10 commits intodevfrom
Issue1516
Closed

implement clearcache command#13
xavierdecoster wants to merge 10 commits intodevfrom
Issue1516

Conversation

@xavierdecoster
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Most strings have a '{0}' and use LocalizedResourceManager to fill in the parameter instead of concatenating the strings like this is doing. I suggest using that instead to keep things consistent and clear.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please break long lines

@emgarten
Copy link
Copy Markdown
Member

emgarten commented Oct 8, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you use nameof instead of using a string for ClearCacheCommand_ClearingNuGetHttpCache. Same comment for other places

@yishaigalatzer
Copy link
Copy Markdown
Contributor

@bradwilson this one is for you

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not return a bool?

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.

Addressed in 2393072

@yishaigalatzer
Copy link
Copy Markdown
Contributor

Please don't rebase before your next commit, so comments don't disappear.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still don't see a fix for this comment. What infrequent exception is being thrown? And why can't we use the .NET implementation?

@xavierdecoster
Copy link
Copy Markdown
Contributor Author

Will close this PR and send a new one to keep track of this discussion.

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.

5 participants