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] Namespaced strings conversions #10980

Merged
merged 1 commit into from
May 15, 2015
Merged

[BUGFIX] Namespaced strings conversions #10980

merged 1 commit into from
May 15, 2015

Conversation

artych
Copy link

@artych artych commented Apr 28, 2015

Fixes #11000

@artych artych changed the title Namespaced strings conversions fix with tests [BUGFIX] Namespaced strings conversions Apr 29, 2015
@fivetanley
Copy link
Member

It's not immediately obvious when I read the code what STRING_CAMELIZE_REGEXP_2 is for. The change itself LGTM but it'd be great for future visits to the code to have something more intention revealing. Reading RegExp's is hard and it's not clear why _2 and not _2 exist

@artych
Copy link
Author

artych commented May 1, 2015

Thank you, @fivetanley. I will clean up code, but I'd like to understand you correctly. There were at least 3 options of coding in that file before this PR:
Option 1 (2 regexp vars):

var STRING_UNDERSCORE_REGEXP_1 = (/([a-z\d])([A-Z]+)/g);
var STRING_UNDERSCORE_REGEXP_2 = (/\-|\s+/g);
var UNDERSCORE_CACHE = new Cache(1000, function(str) {
  return str.replace(STRING_UNDERSCORE_REGEXP_1, '$1_$2').
    replace(STRING_UNDERSCORE_REGEXP_2, '_').toLowerCase();
});

Option 2 (1 regexp var, 1 inline regexp):

var CAMELIZE_CACHE = new Cache(1000, function(key) {
  return key.replace(STRING_CAMELIZE_REGEXP, function(match, separator, chr) {
    return chr ? chr.toUpperCase() : '';
  }).replace(/^([A-Z])/, function(match, separator, chr) {
    return match.toLowerCase();
  });
});

Option 3 (without regexp):

var CLASSIFY_CACHE = new Cache(1000, function(str) {
  var parts = str.split(".");
  var out = [];

  for (var i=0, l=parts.length; i<l; i++) {
    var camelized = camelize(parts[i]);
    out.push(camelized.charAt(0).toUpperCase() + camelized.substr(1));
  }

  return out.join(".");
});

Quite a diverse mix, isn't it? Which option do you suggest me to follow to make this PR code immediately obvious?

@artych
Copy link
Author

artych commented May 3, 2015

Since no response, I've clean the code in favor of option 1, moved regexp vars just before function they used in, added comments. IMHO it looks better now.

@fivetanley
Copy link
Member

LGTM. Can you amend the commit message to be [BUGFIX beta] so it goes out this release?

@stefanpenner r?

Now Camelize, capitalize, classify consider "/".
For example, 'private-docs/owner-invoice'.classify(); // 'PrivateDocs/OwnerInvoice'
@artych
Copy link
Author

artych commented May 5, 2015

@fivetanley [BUGFIX beta] done.

@artych
Copy link
Author

artych commented May 15, 2015

Looks like forgotten during major changes?

@rwjblue
Copy link
Member

rwjblue commented May 15, 2015

Yeah, sorry about that.

rwjblue added a commit that referenced this pull request May 15, 2015
[BUGFIX] Namespaced strings conversions
@rwjblue rwjblue merged commit e6a165e into emberjs:master May 15, 2015
@artych artych deleted the namespaced-strings-fix branch May 15, 2015 17:10
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.

Ember.String wrong conversions for namespaced model name
3 participants