Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Ember.String.humanize #3224

Merged
merged 1 commit into from
Sep 23, 2013
Merged

Add Ember.String.humanize #3224

merged 1 commit into from
Sep 23, 2013

Conversation

sentientmonkey
Copy link
Contributor

Have been using this for creating readable error messages - mirrors rails' implementation.

@alexspeller
Copy link
Contributor

This would be super usefule

of string. Also strips "_id" suffixes.

```javascript
'first_name'.humanize() // 'First Name'
Copy link
Contributor

Choose a reason for hiding this comment

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

This should read // 'First name'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct! Good thing my unit tests are right. Will fix documentation.

@sentientmonkey
Copy link
Contributor Author

@alexspeller Thanks! I agree. I've been using this to try and re-create readable error messages that return back from rails active record errors. I like the return of {errors: {attribute_name: "error messages"}} but I want to return readable error messages in a list (like errors_for in rails views). First thing, though, is getting this into ember.js 😄

@wagenet
Copy link
Member

wagenet commented Sep 10, 2013

Technically this is a new feature and should be updated to follow the guidelines here. However, I think it might be ok to make an exception since this is so small. @wycats?

@wagenet
Copy link
Member

wagenet commented Sep 10, 2013

@sentientmonkey
Copy link
Contributor Author

Ah, sorry - I think I added this pre 1.0.0 final so I didn't follow that convention. Given that it's a new function, and doesn't impact existing functionality (truly a "new feature"), I think it would make sense to bypass the feature flag, but that's your call 😄

Today I'm using this in my own app for easy to read error messages - was going to contribute that to ember-data, but need to sort out upgrading to the beta version first and see how it fits. Either way, it's useful on it's own.

@wagenet
Copy link
Member

wagenet commented Sep 10, 2013

In theory, all new API should be flagged, but this is so tiny it seems funny to flag.

@wagenet
Copy link
Member

wagenet commented Sep 10, 2013

@wycats points out that flags will help us to identify new features, so lets go ahead and flag this anyway. Should be super easy.

@wycats
Copy link
Member

wycats commented Sep 10, 2013

I'd like it if this was flagged. I'm sure it'll sail through the Go/No-Go process.

@wagenet
Copy link
Member

wagenet commented Sep 10, 2013

Also, @sentientmonkey, can you squash the commits when you do that.

@wycats
Copy link
Member

wycats commented Sep 10, 2013

@sentientmonkey the new feature flags are about new features (things that require a semver minor version bump). Breaking changes are disallowed.

@sentientmonkey
Copy link
Contributor Author

@wycats Ok, sounds reasonable - will read through and figure out how to wrap in feature flag. Do you have any recent commits that are a good example to work from?

@wagenet will squash and push commits - I think that plays nicely with github, but never tried ;)

@wagenet
Copy link
Member

wagenet commented Sep 10, 2013

@sentientmonkey It does work. We do it a lot :)

@sentientmonkey
Copy link
Contributor Author

Okay, squashed the commit and wrapped in runtime flag for injecting into String - although should I wrap the declaration as well?

@wagenet
Copy link
Member

wagenet commented Sep 17, 2013

@sentientmonkey Yeah, lets go ahead and wrap Ember.String.humanize in a flag as well since that's also a public API. Also, can you lowercase the feature name, i.e. string-humanize? Thanks!

@sentientmonkey
Copy link
Contributor Author

@wagenet I've pushed a new version with all of the calls wrapped, but it fails now. Where does the features.json go? I can't seem to get that to work properly when my tests run. Sorry for all the turn-around on this one!

@alexspeller
Copy link
Contributor

@sentientmonkey you need to wrap the tests in a feature flag too.

If you rebase against master, you'll find that the tests run with all feature flags enabled now.

@sentientmonkey
Copy link
Contributor Author

I think I figured it out using the browser testing. All wrapped in feature flags for tests and I also added a fix so that it handles multiple words (like first_and_last_name = "First and last name"). I always forget that replace in javascript it's a gsub by default.

@sentientmonkey
Copy link
Contributor Author

Thanks @alexspeller !

wagenet added a commit that referenced this pull request Sep 23, 2013
@wagenet wagenet merged commit b55288a into emberjs:master Sep 23, 2013
@sentientmonkey
Copy link
Contributor Author

Awesome! Thanks!

@stefanpenner stefanpenner mentioned this pull request Jan 3, 2014
8 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants