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

Improving Error Handling: Missing PARTIAL_FILE and long switch-case refactoring #1140

Closed
laxerhd opened this issue Nov 9, 2024 · 2 comments · Fixed by #1142
Closed

Improving Error Handling: Missing PARTIAL_FILE and long switch-case refactoring #1140

laxerhd opened this issue Nov 9, 2024 · 2 comments · Fixed by #1142
Assignees

Comments

@laxerhd
Copy link
Contributor

laxerhd commented Nov 9, 2024

Description

While taking a look at error.h, I noticed that the ErrorCode enum entry PARTIAL_FILE is not used in the error.cpp switch statement. Is this intended?

I also wanted to ask if there was any specific reason for including CURLE_OBSOLETE46 while not adding the other obsolete cURL error codes? Are the others related to unsupported protocols?

PARTIAL_FILE and CURLE_OBSOLETE46 were both added in this commit when adding better cpr error codes in september.

Additionally error.cpp looks kind of scarry with its 200-line switch-case statement. I was wondering if using an unordered_map instead of the switch-case statement might make the code a bit more readable and easier to maintain.

For example:

static const std::unordered_map<std::int32_t, ErrorCode> curl_error_map = {
    {CURLE_OK, ErrorCode::OK},
    {CURLE_UNSUPPORTED_PROTOCOL, ErrorCode::UNSUPPORTED_PROTOCOL},
    // -- AND FOR ERROR CODES ONLY SUPPORTED IN SPECIFIC LIBCURL_VERSION --
#if LIBCURL_VERSION_NUM >= 0x074E00
    {CURLE_SETOPT_OPTION_SYNTAX, ErrorCode::SETOPT_OPTION_SYNTAX},
#endif
    ...
}

Example/How to Reproduce

Files to take a look at: error.cpp and error.h as well as this commit

Possible Fix

  1. Adding PARTIAL_FILE-case to error.cpp
  2. Include/Remove obsolete error codes for consistency
  3. Refactor errror.cpp to use map instead of a switch-case

Where did you get it from?

GitHub (branch e.g. master)

Additional Context/Your Environment

  • OS: -
  • Version: -
@COM8
Copy link
Member

COM8 commented Nov 12, 2024

@laxerhd thanks for bringing this to my attention.

  • PARTIAL_FILE you are right. This is missing. It should map to CURLE_PARTIAL_FILE which was added in curl 7.1
  • CURLE_OBSOLETE46 can be removed. There is no reason to include it.
  • Map based approach: Yes! I really like this approach.

Would you like to create a PR for it?

@laxerhd
Copy link
Contributor Author

laxerhd commented Nov 12, 2024

Yeah, I can do that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants