Skip to content

Refactor out unique assertion - #225

Closed
mattsaxon wants to merge 1 commit into
python-zeroconf:masterfrom
mattsaxon:master
Closed

Refactor out unique assertion#225
mattsaxon wants to merge 1 commit into
python-zeroconf:masterfrom
mattsaxon:master

Conversation

@mattsaxon

Copy link
Copy Markdown
Collaborator

As per code review comments from #217

@mattsaxon mattsaxon mentioned this pull request Feb 3, 2020
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 93.669% when pulling c90cc7b on mattsaxon:master into bef8f59 on jstasiak:master.

Comment thread zeroconf/__init__.py
Comment thread zeroconf/__init__.py
or record.type == _TYPE_AAAA
):
if self.is_type_unique(record.type):
assert record.unique

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We could put the assert in the function/method too and call it assert_unique_used_correctly or something I think.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I considered it, but thought it was more readable as I have it. I pondered it for a while and decided the readability was better than saving the one line. I'd appreciate your view to be honest and could have gone either way.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's fair, looks good either way.

Comment thread zeroconf/__init__.py

@staticmethod
def is_type_unique(type_: int) -> bool:
return type_ == _TYPE_TXT or type_ == _TYPE_SRV or type_ == _TYPE_A or type_ == _TYPE_AAAA

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe just type_ in {_TYPE_TXT, _TYPE_SRV, _TYPE_A, _TYPE_AAAA}?

@jstasiak

Copy link
Copy Markdown
Collaborator

Sorry, I forgot about this pull request – since you pushed the commit as part of #228 I merged it from there and this can be closed.

@jstasiak jstasiak closed this Feb 20, 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