Skip to content

Use ID-based equality check for TemplateFormat consistently #530

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

Closed
wants to merge 2 commits into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Jul 20, 2018

There are three ways to check equality for TemplateFormats constants:

  • getId().equals()
  • equals()
  • ==

As they are constants, == will be sufficient. So this PR changes to use == for equality check consistently.

@wilkinsona
Copy link
Member

wilkinsona commented Jul 20, 2018

Thanks, @izeye. While there are constants for Asciidoctor and Markdown, there's nothing to stop someone creating their own TemplateFormat implementation with the same ID as one of those constants. If someone does that, their TemplateFormat should be considered equal to the built-in one. In other words, getId().equals() is the right comparison to use in main code. It's less clear-cut in the tests as then we know we'll only be dealing with the constants.

@izeye
Copy link
Contributor Author

izeye commented Jul 20, 2018

@wilkinsona Thanks for the feedback! If so, how about implementing TemplateFormat.equals() with TemplateFormat.getId() and using TemplateFormat.equals() consistently?

@wilkinsona
Copy link
Member

With TemplateFormat being an interface, it's difficult to enforce an implementation of equals. I think I'd prefer to keep the existing id-based equality check in the main code. I don't really feel strongly about the tests. Given that the constants exist and they're expected to be used, a same instance check is not unreasonable. On the other hand, lots of the tests shouldn't really know or care about the constants and a id-based equality check would keep things more loosely coupled.

@izeye
Copy link
Contributor Author

izeye commented Jul 20, 2018

@wilkinsona Sorry, I missed the fact that TemplateFormat is an interface. I'm not sure I understood your last comment correctly. Do you prefer an ID-based equality check or just to leave things as they are?

@wilkinsona
Copy link
Member

wilkinsona commented Jul 20, 2018

Sorry, I wasn't very clear. I think the test's current use of equalTo to compare TemplateFormat instances could be improved. It should either use sameInstance (thereby verifying that the constant is being used), or it should compare the formats' IDs. I think I prefer the latter as it minimises any coupling to the constants.

@izeye izeye force-pushed the template-formats branch from 62b1348 to 6bb90d6 Compare July 20, 2018 12:01
@izeye
Copy link
Contributor Author

izeye commented Jul 20, 2018

@wilkinsona Thanks for the clarification! I updated this as you suggested.

@izeye izeye changed the title Use consistent equality check for TemplateFormats constants Use ID-based equality check for TemplateFormat consistently Jul 21, 2018
@izeye
Copy link
Contributor Author

izeye commented Jul 21, 2018

I missed two places and polished them a bit in 4e75e00.

@wilkinsona wilkinsona added this to the 1.2.6.RELEASE milestone Aug 1, 2018
@wilkinsona wilkinsona added the type: task Non user-facing work label Aug 1, 2018
@wilkinsona wilkinsona closed this in d05f6cd Aug 1, 2018
wilkinsona added a commit that referenced this pull request Aug 1, 2018
* gh-530:
  Polish "Consistently use ID-based equality check for TemplateFormat"
  Consistently use ID-based equality check for TemplateFormat
@wilkinsona
Copy link
Member

Thanks again, @izeye. These changes are now in 1.2.x and master.

@izeye izeye deleted the template-formats branch August 1, 2018 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task Non user-facing work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants