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

[FEATURE ember-metal-is-present] Add Ember.isPresent #5136

Merged
merged 1 commit into from
Jul 10, 2014

Conversation

amiel
Copy link
Contributor

@amiel amiel commented Jul 9, 2014

Maybe it's because I come from rails, but I miss having .present? as the inverse of .blank?. I find that I end up using ! Ember.isBlank() more than Ember.isBlank() by itself. I know this is a simple change, but I think the concept of presence is useful and more semantic. I first thought of this when working on jamesarosen/ember-cpm#36, and keep on finding times where it would make my code easier to understand.

@stefanpenner
Copy link
Member

this is a new feature, it must follow those guidelines: see http://emberjs.com/guides/configuring-ember/feature-flags/

A value is present if it not `isBlank`.

```javascript
Ember.isPresent(); // false
Copy link
Member

Choose a reason for hiding this comment

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

There is no test case for this scenario ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pangratz Thanks, I've added a test.

@amiel amiel changed the title Add Ember.isPresent [FEATURE isPresent] Add Ember.isPresent Jul 9, 2014
@amiel
Copy link
Contributor Author

amiel commented Jul 9, 2014

@stefanpenner Sorry about that. I looked through CONTRIBUTING.md but not http://emberjs.com/guides/contributing/adding-new-features/.

Is guarding the import and Ember.isPresent = isPresent sufficient? or do I need to guard around the implementation and test as well. Thanks!

@stefanpenner
Copy link
Member

@amiel ya, someone should add that to contributing.MD as well (mind doing it?)

@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2014

@amiel - Would also be nice to have the commit prefixes in there (see #5135 (comment))...

@amiel
Copy link
Contributor Author

amiel commented Jul 9, 2014

Please see #5137

@amiel amiel changed the title [FEATURE isPresent] Add Ember.isPresent [FEATURE ember-metal-is-present] Add Ember.isPresent Jul 9, 2014
@amiel
Copy link
Contributor Author

amiel commented Jul 9, 2014

hmmm, it looks like this failed at npm install :(

@amiel
Copy link
Contributor Author

amiel commented Jul 9, 2014

It looks like the failure now is in defeaturify; am I doing something wrong with the feature flags?

@@ -9,6 +9,7 @@
"ember-routing-will-change-hooks": null,
"ember-routing-consistent-resources": true,
"event-dispatcher-can-disable-event-manager": null
"ember-metal-is-present": null,
Copy link
Member

Choose a reason for hiding this comment

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

Trailing commas are not valid JSON.

@amiel
Copy link
Contributor Author

amiel commented Jul 9, 2014

@rjackson thanks. it looks like I had a couple of other feature flag issues as well (I didn't know import and export had to live outside my conditionals). I think they are all fixed, and the CI build just needs to be restarted again because of the npm issues :(

@trek
Copy link
Member

trek commented Jul 10, 2014

LGTM!

@rwjblue
Copy link
Member

rwjblue commented Jul 10, 2014

@amiel - Can you add an entry to FEATURES.md also (just include a quick sentance or two and the the link to this PR)?

Other than that 👍.

Add Ember.isPresent as inverse of Ember.isBlank
@amiel
Copy link
Contributor Author

amiel commented Jul 10, 2014

Ok, done :)

trek added a commit that referenced this pull request Jul 10, 2014
[FEATURE ember-metal-is-present] Add Ember.isPresent
@trek trek merged commit 02d69c8 into emberjs:master Jul 10, 2014
@amiel amiel deleted the isPresent branch July 10, 2014 18:16
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.

5 participants