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

Allow base64 content for create/update file #1488

Merged
merged 2 commits into from Nov 21, 2016
Merged

Conversation

@laedit
Copy link
Contributor

@laedit laedit commented Oct 6, 2016

Fixes #1143.
Since 1.0 has not been reached yet, I made the fix minimal and as I think the method should have been originally, but there is a breaking change.

If you want to mitigate the breaking change I propose three ways:

  1. Content contains base64 content. Breaking change on Content.Get(). Could add corresponding ctors with bool if convert to base64.
  2. Insert base class Base64CreateFileRequest with a virtual Content property wich contains base64 content. CreateFileRequest overrides it and provides decoded content.
  3. Add SerializeIgnore and SerializeName attributes, add first on Content and second on new property Base64Content. When set, Content convert value to base64 and set Base64Content.
@@ -106,7 +106,7 @@ public class CreateFileRequest : ContentRequest
/// Creates an instance of a <see cref="CreateFileRequest" />.
/// </summary>
/// <param name="message">The message.</param>
/// <param name="content">The content.</param>
/// <param name="content">The content encoded in base64.</param>

This comment has been minimized.

@shiftkey

shiftkey Oct 7, 2016
Member

@ryangribble @haacked how do we feel about this breaking change? Should we instead introduce overloads?

This comment has been minimized.

@haacked

haacked Oct 7, 2016
Contributor

Yeah, that's a very subtle breaking change. Probably best to use an overload.

Someday I'd like to write a Base64Content type. 😄 rather than using strings for everything.

@laedit
Copy link
Contributor Author

@laedit laedit commented Oct 7, 2016

@shiftkey @haacked is that what you have in mind? Or do you prefer another way?

@ryangribble
Copy link
Contributor

@ryangribble ryangribble commented Oct 8, 2016

+1 on not making a breaking change

The overload approach you've gone with seems ok to me 👍

@ryangribble
Copy link
Contributor

@ryangribble ryangribble commented Nov 21, 2016

Sorry for the delay @laedit

This LGTM and the integration tests are 👌 plus the revisions make it a non breaking change 😀

Thanks for the contribution!
LGTM

@ryangribble ryangribble merged commit 88e5342 into octokit:master Nov 21, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laedit
Copy link
Contributor Author

@laedit laedit commented Nov 21, 2016

@ryangribble No problem, thanks for the merge 😄

@laedit laedit deleted the laedit:Allow-base64-content-on-create-file branch Nov 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.