Skip to content
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

Added approximately alias to close to #527

Merged
merged 1 commit into from Oct 4, 2015

Conversation

lexi-sh
Copy link

@lexi-sh lexi-sh commented Oct 4, 2015

Solves #525 , barring some questions.

  1. Should approximately be added to the assert interface as well? This PR does, but this can be removed if you feel it bloats the interface.
  2. The aliasing syntax doesn't allow for custom error messaging. This is fine for things like satisfy vs satisfies, but it gets a bit weird when it's something like closeTo and approximately. I edited the internal error messages to be more explicit ('the arguments to closeTo or approximately must be numbers'), but left the assertion messaging the same as to not break anything that might rely on that specific messaging. I don't really like this, because the closeTo function now knows how it's being used... but not having it can be really confusing for people trying to use approximately in a wrong way. What do you think of this? We could also just leave all messaging the same and treat approximately as a true alias.

@keithamus
Copy link
Member

  1. assert should be a first class citizen - ideally it would have every method the other two interfaces have, including aliases. So we can keep this, good job!
  2. The precedent has been set with other assertion methods - we provide aliases but the messaging always leans on the original method name, but this could be documented better on our website, which is in much need of love anyway. Having said that, I think we can keep the early error regarding type info.

All is good, thanks for the hard work 😄

keithamus added a commit that referenced this pull request Oct 4, 2015
Added approximately alias to close to
@keithamus keithamus merged commit 6d0ae35 into chaijs:master Oct 4, 2015
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.

None yet

2 participants