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

Clean up _parse_uri_s3x function, resolve edge cases #237

Merged
merged 3 commits into from
Oct 1, 2018
Merged

Clean up _parse_uri_s3x function, resolve edge cases #237

merged 3 commits into from
Oct 1, 2018

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented Oct 1, 2018

Fix #195

fix host:port:junk edge case, improve exception handling, simplify logic
@menshikh-iv
Copy link
Contributor

menshikh-iv commented Oct 1, 2018

Looks good @mpenkov!

Can you also fix README.md please, goal - avoid misunderstanding.

@menshikh-iv menshikh-iv merged commit 7ac3b4e into piskvorky:master Oct 1, 2018
@menshikh-iv
Copy link
Contributor

@mpenkov 🔥

@piskvorky
Copy link
Owner

piskvorky commented Oct 1, 2018

Strong -1 on the README changes. 1234 tells me nothing as a reader, port better. A person who doesn't understand that host, port, secret or user are placeholders is not going to be helped by seeing 1234 (in fact, probably even more confused, since they'll just type 1234 verbatim).

It's also inconsistent with keeping the rest as placeholders (host, user, secret…). Why is one concrete and the other general? Should we replace them all by a concrete "mock" value?

@mpenkov mpenkov deleted the 195 branch October 1, 2018 07:19
@mpenkov
Copy link
Collaborator Author

mpenkov commented Oct 1, 2018

@piskvorky The problem with specifying host:port as an example is that it causes misunderstandings like #195. People try the examples verbatim and become surprised when things don't work.

I can see your side of the argument too, however. What's the best way to deal with this? Should we just add text that makes it obvious that strings like host, port, etc. are placeholders and need to be replaced with actual values?

@piskvorky
Copy link
Owner

piskvorky commented Oct 1, 2018

Nah. Our audience here is technical. In my experience, a person who thinks host:port should be taken literally won't really be reading or understanding such additional texts anyway. And they'll definitely copy-paste 1234 too, so that still doesn't help.

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