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

feat: add lengthEq and lengthNotEq #552

Merged

Conversation

Undistraction
Copy link
Collaborator

Adds remaining functions to test of list or string length is equal or notEqual to the supplied length.

Note: I'm unsure of the correct behaviour for lengthNotEq when the value supplied does not have a length property. I am returning false, but should we do something else?

Closes #444

@char0n
Copy link
Owner

char0n commented May 13, 2018

CI is complaining about some jsdoc problems.

Note: I'm unsure of the correct behaviour for lengthNotEq when the value supplied does not have a length property. I am returning false, but should we do something else?

IMHO is absolutely ok to return false in cases like these.

@Undistraction
Copy link
Collaborator Author

CI is complaining about some jsdoc problems.

It's strange - npm run lint doesn't pick up those for me locally. Anyway they are now fixed.

IMHO is absolutely ok to return false in cases like these.

OK.

@Undistraction
Copy link
Collaborator Author

@char0n Btw don't feel you have to review this if you want to get a release out today. It can wait for the next release - just wanted to close out the remaining functions in that issue and thought it made sense for them all to go in the same release. But I don't think anyone will lose sleep if they don't.

src/lengthEq.js Outdated
* @param {number} valueLength The length of the list or string
* @param {number} value The list or string
* @return {boolean}
* @see {@link http://ramdajs.com/docs/#equals|equals},
Copy link
Owner

Choose a reason for hiding this comment

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

@see is on multiple lines, should be on one line, or prettier is doing this?

src/lengthEq.js Outdated
* @category List
* @sig Number -> [*] -> Boolean
* @param {number} valueLength The length of the list or string
* @param {number} value The list or string
Copy link
Owner

Choose a reason for hiding this comment

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

@param {Array|string} value ...

This also apply to all the length* functions that you did before. Include changes to these function into this PR.

* @param {number} valueLength The length of the list or string
* @param {number} value The list or string
* @return {boolean}
* @see {@link http://ramdajs.com/docs/#equals|equals},
Copy link
Owner

Choose a reason for hiding this comment

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

Again on multiple lines.

* @category List
* @sig Number -> [*] -> Boolean
* @param {number} valueLength The length of the list or string
* @param {number} value The list or string
Copy link
Owner

Choose a reason for hiding this comment

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

@param {Array|string} value ...

This also apply to all the length* functions that you did before. Include changes to these function into this PR.

test/lengthEq.js Outdated
import eq from './shared/eq';

describe('lengthEq', function() {
it(`should return true if the length of a list is equal to the supplied length`, function() {
Copy link
Owner

Choose a reason for hiding this comment

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

context('when the length of a list is equal to the supplied length', function() {
  specify('should return true', function() {
    ...
  });
});

test/lengthEq.js Outdated
eq(RA.lengthEq(0, []), true);
});

it(`should return true if the length of a string is equal to the supplied length`, function() {
Copy link
Owner

Choose a reason for hiding this comment

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

context('the length of a string is equal to the supplied length', function() {
  specify('should return true', function() {
    ...
  });
});

test/lengthEq.js Outdated
eq(RA.lengthEq(0, ''), true);
});

it(`should return false for values without a length property`, function() {
Copy link
Owner

Choose a reason for hiding this comment

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

context('when there are values without a length property', function() {
  specify('should return false', function() {
    ...
  });
});

Copy link
Owner

Choose a reason for hiding this comment

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

Do the same for lensNotEq tests.

@char0n
Copy link
Owner

char0n commented May 13, 2018

@Undistraction to late, I already did it ;] It looks nice, we need just need to do some improvements here. I need length* function next week (have a heavy usecases for them) so I want to release today or tomorrow. I think we can get this last PR in easily.

@Undistraction
Copy link
Collaborator Author

I've also added @see refs to each of the other length… functions to each length function.

Copy link
Owner

@char0n char0n left a comment

Choose a reason for hiding this comment

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

Looks good! One note: you're using string template literals to provide descriptions for tests. I didn't even know that our codestyle allows that. This means that we are not consistent with the way we write our tests. Rest of the team is using normal strings to describe tests. We must do something about it.

Update:
I confirm, you started to use this convention on RA.move and everything after it.

@Undistraction
Copy link
Collaborator Author

Undistraction commented May 13, 2018

Ah sorry. I use literals for everything as it means I can interpolate into stings without changing the delimiters which is quicker and better for seeing changes in diffs so I'm doing it out of habit. Obviously I'll try and remember to use normal delimeters but there should be a way of longing for it.

I'll put together a PR switching to normal delimeters and look at how to lint for my use of backticks.

@char0n
Copy link
Owner

char0n commented May 13, 2018

@Undistraction #553

This PR can go inside. We'll deal with the string literals in #553.

@char0n char0n merged commit f71ad1e into char0n:master May 13, 2018
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