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

Allow Number type in updateIn function #67

Merged
merged 2 commits into from
Oct 7, 2016
Merged

Allow Number type in updateIn function #67

merged 2 commits into from
Oct 7, 2016

Conversation

avevlad
Copy link
Contributor

@avevlad avevlad commented Oct 7, 2016

Test case:

u.updateIn(0, 123, [1, 2, 3]);

Before output:

     TypeError: path.split is not a function
      at splitPath (lib/util/splitPath.js:7:17)

After:

[123, 2, 3]


export default function splitPath(path) {
return Array.isArray(path) ?
path :
reject(path.split('.'), x => !x);
reject(toString(path).split('.'), x => !x);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need all of the things that lodash/toString does, can you just use (path + '') instead please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

const path = 'bar.0.y';
const result = splitPath(path);
expect(result).to.deep.equal(['bar', '0', 'y']);
});
Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks for the tests, though could you follow the existing style of tests, specifically:

  • blank lines between tests
  • complete sentences (when "it" is prepended) for the test names, e.g. it('treats a number as a single step path')

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're right, ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronjensen

describe('splitPath', () => {
  it('treats only number', () => {
    const path = 1;
    const result = splitPath(path);
    expect(result).to.deep.equal(['1']);
  });

  it('treats only number', () => {
    const path = 0;
    const result = splitPath(path);
    expect(result).to.deep.equal(['0']);
  });

  it('treats simple array', () => {
    const path = ['foo', 'bar', 'x'];
    const result = splitPath(path);
    expect(result).to.equal(path);
  });

  it('treats a number as a single step path', () => {
    const path = 'bar.0.y';
    const result = splitPath(path);
    expect(result).to.deep.equal(['bar', '0', 'y']);
  });
});

so, it's ok? 😃

Copy link
Member

Choose a reason for hiding this comment

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

sorry, my example was meant to be for the first.

You can certainly simplify, but I would use the word "handles" instead of "treats", since "treats" requires something like "as a ..." at the end of the sentence.

So, "it handles numbers", "it handles arrays", "it handles strings separated by dots"

You can probably get rid of either the 0 or the 1 test, you don't really need them both and they'd have the same 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.

@aaronjensen

import { expect } from 'chai';
import splitPath from '../../lib/util/splitPath';

describe('splitPath', () => {
  it('treats a number as a single step path', () => {
    const path = 1;
    const result = splitPath(path);
    expect(result).to.deep.equal(['1']);
  });

  it('handles arrays', () => {
    const path = ['foo', 'bar', 'x'];
    const result = splitPath(path);
    expect(result).to.equal(path);
  });

  it('handles strings separated by dots', () => {
    const path = 'bar.0.y';
    const result = splitPath(path);
    expect(result).to.deep.equal(['bar', '0', 'y']);
  });
});

Copy link
Member

Choose a reason for hiding this comment

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

looks great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronjensen done :)

@aaronjensen aaronjensen merged commit bc00fee into substantial:master Oct 7, 2016
@aaronjensen
Copy link
Member

Looks great, thank you for the contribution @avevlad

@avevlad
Copy link
Contributor Author

avevlad commented Oct 7, 2016

@aaronjensen thanks you! 👍

@aaronjensen
Copy link
Member

@avevlad released 1.0.0, thanks again

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