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

[BUGFIX release-1-13] Fix string classify normalizer #12383

Merged
merged 2 commits into from
Sep 29, 2015

Conversation

skeate
Copy link
Contributor

@skeate skeate commented Sep 24, 2015

Resolves #11326. If a piece of a name starts with dash (-), it will be changed into an underscore (_).

@skeate skeate changed the title [BUGFIX beta] Fix string classify normalizer [BUGFIX release-1-13] Fix string classify normalizer Sep 24, 2015
}).replace(STRING_CLASSIFY_REGEXP_2, function(match, separator, chr) {
return str
.split('/')
.map(function(piece) {
Copy link
Member

Choose a reason for hiding this comment

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

We can't use Array.prototype.map in release-1-13 (because it must support ES3 browsers), can you swap this to a for loop instead to allow this to be pulled into those branches?

@skeate
Copy link
Contributor Author

skeate commented Sep 26, 2015

Changed.

@rwjblue
Copy link
Member

rwjblue commented Sep 29, 2015

Can you also add tests for these scenarios:

  • classify('_FooBar') === '_FooBar'
  • classify('_Foo_Bar') === '_FooBar'
  • classify('_Foo-Bar') === '_FooBar'
  • classify('_foo/_bar') === '_Foo/_Bar'
  • classify('-foo/_bar') === '_Foo/_Bar'
  • classify('-foo/-bar') === '_Foo/_Bar'

@skeate
Copy link
Contributor Author

skeate commented Sep 29, 2015

Any issue with redoing all these tests as a sort of macro function? i.e. they're all like

QUnit.test('classify normal string', function() {
  deepEqual(classify('my favorite items'), 'MyFavoriteItems');
  if (Ember.EXTEND_PROTOTYPES) {
    deepEqual('my favorite items'.classify(), 'MyFavoriteItems');
  }
});

-- could write one function

function test(description, given, expected) {
  QUnit.test(description, function() {
    deepEqual(classify(given), expected);
    if (Ember.EXTEND_PROTOTYPES) {
      deepEqual(given.classify(), expected);
    }
  });
}

and call it for all test cases.

Would be easier to write them and easier to see all the different test cases.

[Edit: and if so, should I make that a separate commit or amend it?]

@rwjblue
Copy link
Member

rwjblue commented Sep 29, 2015

Ya, one commit with the fix and new tests (the old way), and a second commit that refactors to a saner structure sounds great.

@rwjblue
Copy link
Member

rwjblue commented Sep 29, 2015

Only the first commit would be [BUGFIX release-1-13] though. The other would be [CLEANUP canary] I think.

@skeate
Copy link
Contributor Author

skeate commented Sep 29, 2015

alright, have it done in a min

Resolves emberjs#11326. If a piece of a name starts with dash (-), it will be
changed into an underscore (_).
@rwjblue
Copy link
Member

rwjblue commented Sep 29, 2015

Looks great, thank you!

rwjblue added a commit that referenced this pull request Sep 29, 2015
[BUGFIX release-1-13] Fix string classify normalizer
@rwjblue rwjblue merged commit aefcf14 into emberjs:master Sep 29, 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.

2 participants