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

Return error when default URL cannot be parsed #205

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

Conversation

tty2
Copy link
Contributor

@tty2 tty2 commented Dec 8, 2020

This changes are more as a proposal.

I see the point why this error check is not so important (we parse default url), but error check can help if someone changes the default url wrong way. I know that it's extremely rare case but I can't see any advantage to skip the error check on init. It will not save time and even if it will do, this saved time will be so tiny and it will not have any value because it's only on init elastic (once I mean).

WDYT?

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@karmi
Copy link
Contributor

karmi commented Dec 9, 2020

@tty2 , principially, I see no problem with returning an error in this place — the short-circuiting was motivated just by keeping the code more succinct. Can you check what kind of error is returned/displayed in this case? Should we include "cannot parse default URL" in the error message (also see the capitalization). Also, can you please also change the commit message to something more concrete, eg. Return error when default URL cannot be parsed?

(Note that I'm no longer maintaining the package, so can't process the PR itself.)

@tty2
Copy link
Contributor Author

tty2 commented Dec 9, 2020

Hi, @karmi. Thanks for the answer.
Sad to know that you are not a maintainer any more. Thanks for your work and for this client. I use it in my projects and like it.

About your comments:

Can you check what kind of error is returned/displayed in this case?

as example:

Unexpected error: cannot parse default URL: parse "cache_object://localhost:9200": first path segment in URL cannot contain colon

Should we include "cannot parse default URL" in the error message (also see the capitalization).

So we should change the error message in this line as well. I mean capitalize url word.

@tty2 tty2 changed the title check error on elastic init Return error when default URL cannot be parsed Dec 9, 2020
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.

3 participants