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

Fix doc for model.has return values #1712

Merged
merged 1 commit into from Dec 22, 2017

Conversation

Projects
None yet
2 participants
@adamjaffeback

adamjaffeback commented Dec 22, 2017

Fix doc for model.has return values

  • Related Issues: none
  • Previous PRs: none

Introduction

model.has(attribute) returns true when the attribute is set. If the attribute is not set, the function returns false. The doc incorrectly states that the falsy returned value is null.

otherwisenull

Motivation

It is generally considered best practice in JavaScript to use === instead of == when comparing values to ensure that automatic type conversion doesn't cause unintended consequences. While using model.has(attribute), I was looked specifically for the function to return null (per docs) if the attribute was not set:

// if deleted_on attribute is not set
if (model.has('deleted_on') === null) {
  // set it
  model.set('deleted_on', moment(MAX_DATE).format('Y-MM-DD HH:mm:ss'));
}

As a result of the documentation error, I never get into the if-statement's body, because model.has(attribute) never returns null. It always returns a boolean value as a result of

return this.get(attr) != null;

Proposed solution

My solution is to update the documentation to reflect the current state of the source code. model.has(attribute) always returns a boolean, which is correctly documented by this PR.

Current PR Issues

None.

Alternatives considered

I also considered adding to the description statement (below), but found that the change to the Returns section was sufficient.

bookshelf/src/base/model.js

Lines 319 to 320 in fd10651

* @description
* Returns `true` if the attribute contains a value that is not null or undefined.

If the function returns true when an attribute is set, users quickly infer that it will return false if the attribute is not set. The documentation doesn't need to repeat this information twice, once in the Returns section, which this PR changes, and once in the description.

Adam Jaffe Back

@ricardograca ricardograca self-requested a review Dec 22, 2017

@ricardograca ricardograca added the docs label Dec 22, 2017

@ricardograca

This comment has been minimized.

Member

ricardograca commented Dec 22, 2017

I'm pretty sure the test didn't fail because of this change.

@ricardograca

Looks good to me. We should really have a standard on how to format values, but not a big problem right now.

@ricardograca ricardograca merged commit ed0d726 into bookshelf:master Dec 22, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@adamjaffeback

This comment has been minimized.

adamjaffeback commented Dec 22, 2017

@ricardogama thanks for the merge. I agree with you, the only failure in tests was during the installation of oracledb (link to Travis CI logs).

It only fails against Node 7...and it doesn't look like it was just me; it started popping up 4 days ago on #1710. This is related to oracle/node-oracledb#811. I'll open an issue, as it looks like this won't be going away.

@adamjaffeback adamjaffeback deleted the adamjaffeback:patch-1 branch Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment