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

Client option to log full reason for HTTP errors #319

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

RiskoZoSlovenska
Copy link
Contributor

This pull request adds a new boolean client option logFullErrors (true by default) which, when true, makes the API class log the full error reason (found inside the response body) of requests that return an error response. This makes debugging much easier as you're able to see exactly what went wrong without having to manually print the error message returned by Discordia methods.

As an example, what before was:

2021-10-23 15:03:39 | [ERROR]    | 400 - Bad Request : POST https://discord.com/api/v8/channels/901541671806857238/messages

now becomes:

2021-10-23 15:03:39 | [ERROR]    | 400 - Bad Request : POST https://discord.com/api/v8/channels/901541671806857238/messages
HTTP Error 50035 : Invalid Form Body
        BASE_TYPE_REQUIRED in payload.embeds[0].fields[0].value : This field is required

@SinisterRectus
Copy link
Owner

Going to wait on this one, but not deny it.

@SinisterRectus
Copy link
Owner

Good enough for 2.x. I can always rewrite it in 3.0. I would rather have this as false by default though.

@object-Object
Copy link
Contributor

It'd be nice for debugging if a file & line number could be added to this, if that's easy to do.

@Bilal2453
Copy link
Contributor

It'd be nice for debugging if a file & line number could be added to this, if that's easy to do.

That is definitely doable with the help of the debug library, making use of getinfo I would guess, but I don't know if it is worth it. Usually using the debug library has some serious performance implications, and calling this on every request error... that might be a bit too much.

@object-Object
Copy link
Contributor

Even if it's disabled by default like Sinister said?

@Bilal2453
Copy link
Contributor

There will always be http errors you cannot avoid. But in the case it was false by default and be turned on explicitly by the user, and the user was smart about it and didn't turn it on all of the time, it can be useful indeed. Perhaps, if it had its own client option?

Nonetheless, in my opinion this is something we can think of for Discordia 3.

@RiskoZoSlovenska
Copy link
Contributor Author

I would rather have this as false by default though.

Done; 7d7f163

It'd be nice for debugging if a file & line number could be added to this, if that's easy to do.

Agreed, but as @Bilal2453 mentioned, I think it would be best for it to be its own client option.

@SinisterRectus
Copy link
Owner

Good enough for 2.x. I can always rewrite it in 3.0. I would rather have this as false by default though.

I just realized this PR is for 3.0. Awkward.

@SinisterRectus
Copy link
Owner

I honestly don't like the code. In particular: the concatenation before calling API:log then the potential interpolation of an empty string into %s in API:log.

@RiskoZoSlovenska
Copy link
Contributor Author

RiskoZoSlovenska commented Jan 22, 2022

Understandable. Would adding something like this be better?

function API:logWithMessage(level, res, method, url, message)
	return self._client:log(level, '%i - %s : %s %s\n%s', res.code, res.reason, method, url, message)
end

My worry is that this duplicates code.

@RiskoZoSlovenska
Copy link
Contributor Author

...or perhaps something like this?

function API:log(level, res, method, url, extra)
	local base = self._client:log(level, '%i - %s : %s %s', res.code, res.reason, method, url)
	
	if extra ~= nil then
		return base .. '\n' .. extra
	else
		return base
	end
end

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.

4 participants