Skip to content

Conversation

nikell28
Copy link

No description provided.

marazmiki and others added 4 commits October 10, 2021 15:49
* Fixed a sad misspell in bumpversion config
* Added Python 3.10 binary for GitHub actions
* Trying to make coveralls work
docs: clarified the documentation
fix: type hintings
Copy link
Owner

@marazmiki marazmiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix some issues described inside the review?


master_doc = "index" # It's important for RTFD
exclude_patterns = []
exclude_patterns: list = []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need to hint sphinx config :)

self._dsn = dsn
self._raw_bits = None
self._parsed_bits = {}
self._raw_bits: ParseResult
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not sure we should type-hint any internals.

return self.default_region

def _parse_bucket_name(self) -> str:
def _parse_bucket_name(self) -> Optional[str]:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, this shouldn't be an optional one.

return super()._is_host_given()

def _parse_bucket_name(self) -> str:
def _parse_bucket_name(self) -> Optional[str]:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I cannot even imagine the situation, when BUCKET is not presented inside S3 URLs. So, I'm pretty sure, everything related to a bucket should be mandatory rather than optional

@marazmiki marazmiki force-pushed the master branch 3 times, most recently from 20eb25e to c943d86 Compare July 31, 2022 16:00
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.

2 participants