Skip to content

Fix condision for error when duplicate link titles#47

Merged
cyberdelia merged 2 commits into
interagent:masterfrom
theoden9014:master
Jun 2, 2017
Merged

Fix condision for error when duplicate link titles#47
cyberdelia merged 2 commits into
interagent:masterfrom
theoden9014:master

Conversation

@theoden9014
Copy link
Copy Markdown
Contributor

CheckForDuplicateTitles() returns true if there are duplicates.
Therefore, if it is not duplicated, an error occurred.

@wchrisjohnson
Copy link
Copy Markdown
Contributor

Given the recent schema updates/cleanup, the generator produces a correct client either way; there are currently no duplicates in the schema - yay!

With that said, as the author of this recent addition to the generator, and on reading it now a month or so removed from the change, I am less than comfortable with it readability-wise. If I may suggest, let's:

  • change the name of the method to something like AreTitleLinksUnique, and
  • change the return to something like return len(uniqueLinks) == len(s.Links)
  • leave your update above the same (except change the method being called)

That way we are stating exactly what we expect to be true and the code checks for a match with that expectation.

@theoden9014 @cyberdelia how doe that sound?

@theoden9014
Copy link
Copy Markdown
Contributor Author

theoden9014 commented Jun 2, 2017

Thank you for your quick reply !

I agree with your idea, It's hard to understand what's returned if the method name is CheckFor.

@wchrisjohnson
Copy link
Copy Markdown
Contributor

👍 LGTM

@cyberdelia cyberdelia merged commit 4828431 into interagent:master Jun 2, 2017
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