Allow using '@' in object (key) names #204
Conversation
|
Sure, I'll add them today. |
dkasyanov
changed the title
|
@mpenkov I've added tests. |
There was a problem hiding this comment.
Awesome! Thank you for adding test code. Could you please split the code into new test cases? Looking at your changes, I see 3 separate test cases:
- test_allows_atmark_in_key (names for example only)
- test_allows_atmark_in_key_and_credentials
- test_invalid_atmark
We don't want a single test testing too many things, because if such a test fails, it can hide multiple failures. Besides, if it tests multiple things it's not really a unit test.
| @@ -83,7 +83,28 @@ def test_s3_uri(self): | |||
| self.assertEqual(parsed_uri.access_id, "accessid") | |||
| self.assertEqual(parsed_uri.access_secret, "access/secret") | |||
|
|
|||
| # incorrect uri - only two '@' in uri are allowed | |||
| # correct uri, contains credentials and '@' in object name | |||
There was a problem hiding this comment.
This should be a separate test.
| self.assertEqual(parsed_uri.access_secret, "access/secret") | ||
|
|
||
| # correct uri, contains credentials, host, port and '@' in object name | ||
| parsed_uri = smart_open_lib._parse_uri("s3://accessid:access/secret@hostname:1234@mybucket/dir/my@ke@y") |
There was a problem hiding this comment.
This should be a separate test.
| self.assertEqual(parsed_uri.host, "hostname") | ||
| self.assertEqual(parsed_uri.port, 1234) | ||
|
|
||
| # incorrect uri - bucket can't contain '@' |
There was a problem hiding this comment.
This should be a separate test.
|
ping @dkasyanov, are you planning to finish PR? |
|
@menshikh-iv We can probably merge this and clean up the tests as a separate ticket. My main issue was that each of the new unit tests tests multiple things, which is bad unit testing practice. Alternatively, I can finish this PR myself. Let me know which you prefer. |
|
@mpenkov it will be nice if you finish PR yourself |
|
Continued in #224 |
Related to the #94 issue.
Keys (objects) in AWS S2 may contain multiple '@'.
So it may be more than 2 '@' in the URI.
P.S. Bucket names can contain only lowercase letters, numbers, and hyphens.