-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
bugfix #2870 #2871
base: main
Are you sure you want to change the base?
bugfix #2870 #2871
Conversation
Hmm...I'm uncomfortable making this change with such a broad impact. Can it be scoped down to the one request necessary? |
@kfcampbell , I agree that this can be dangerous. I will look at a way to narrow this fix to the delete request and make it available to other kind of request if the issue appears on other. Thanks for your comment |
@b3b00 if you get the chance post what your use (code) of the SDK here looks like - specifically I am trying to find out which resource client you're using so that we can help you craft a solution. We have some mechanisms in place to support adding headers to the request - adding where you have will definitely have some side effects for all of the other implementations that use |
Hello @nickfloyd, my code : public override async Task DeleteNote(string noteName)
{
if (gitHubClient != null)
{
var content = await NoteExists(noteName);
if (content.exists)
{
// this is were the exception happens
var deleteRequest =
new DeleteFileRequest($"DendrOnline : delete note {noteName}", content.content.Sha);
await gitHubClient.Repository.Content.DeleteFile(RepositoryId,$"notes/{noteName}.md" , deleteRequest);
}
}
} |
Resolves #2870
Before the change?
When trying to delete a file an exception is raised stating
System.ArgumentException: The value cannot be null or empty. (Parameter 'mediaType')
. And the file is not deletedAfter the change?
No exception is raised and the file is deleted.
Pull request checklist
Does this introduce a breaking change?