search

haraka / node-address-rfc2822

Parser for RFC2822 (Header) format email addresses
https://www.npmjs.com/package/address-rfc2822
53 stars 16 forks source link

Extracting non-latin names (proper check for uppercase) #9

Closed vladholubiev closed 8 years ago

vladholubiev commented 8 years ago

fixes #8

vladholubiev commented 8 years ago

Travis build fails because of linting.

Missing version constraints in package.json "eslint": "*" - so it install latest version which is incompatible with current grunt-eslint

vladholubiev commented 8 years ago

I disabled linting in .travis.yml as it requires to fix code style in whole project to make ESLint pass. And this is irrelevant to current PR

msimerson commented 8 years ago

Hi @vladgolubev , thanks for the PR. Please just remove the 2nd commit, it disables lint checks entirely and that's not what we want. I'll fix the lint issues in another PR.

vladholubiev commented 8 years ago

@msimerson Reverted 2nd commit

msimerson commented 8 years ago

thanks. I'm fixing the lint issues. Once done, I should be able to pull in this branch and get passing tests.

vladholubiev commented 8 years ago

@msimerson

  1. Agree!
  2. Oh, that's a holy war topic :smiley: I adhere this approach - https://github.com/sindresorhus/ama/issues/10#issuecomment-117766328
  3. +
  4. I checked that module, yes it uses native toUpperCase(). I didn't dig into details, but I see it adds some fallbacks for edge cases, probably because toUpperCase() (as whole JavaScript itself, TBH) has flaws. I believe author of the module did some research before writing smth such trivial. And 1,326,766 downloads in last month are perhaps not causeless.
  5. Also agree. That's the most valid point among above, so I don't mind :+1:

It's your module after all, so last decision is up to you.

smfreegard commented 8 years ago

@vladgolubev - the question is, with this change that @msimerson has proposed to your original code; does it still work for your edge-case?

vladholubiev commented 8 years ago

@smfreegard yes it is

smfreegard commented 8 years ago

Ok +1 then on not introducing additional dependencies unless we have good reason to.

msimerson commented 8 years ago
  1. I checked that module, yes it uses native toUpperCase(). I didn't dig into details, but I see it adds some fallbacks for edge cases, probably because toUpperCase() (as whole JavaScript itself, TBH) has flaws. I believe author of the module did some research before writing smth such trivial. And 1,326,766 downloads in last month are perhaps not causeless.

Excellent point, about the edge cases. But, before worrying about them edge cases, I want them defined and understood. The best tool we have to doing that is our test suite, so if an edge case can be defined, please please open an issue or PR that adds the case(s) to the test suite. Maybe even switch to using upper-case. In any case, having the tests will avoid breaking functionality down the road.