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

Added undefined as required #65

Merged
merged 1 commit into from
Apr 23, 2018
Merged

Added undefined as required #65

merged 1 commit into from
Apr 23, 2018

Conversation

r-khr
Copy link
Contributor

@r-khr r-khr commented Apr 21, 2018

@MrWolfZ
Copy link
Owner

MrWolfZ commented Apr 21, 2018

Thank you for this PR. Please rebase your change onto develop and target develop with this PR (you'll have to perform a npm install since the dependencies have changed on develop compared to master). Also please update the inline comment of the required function to mention that the value must be non-undefined (i.e. adjust line 4 in required.ts).

@r-khr r-khr changed the base branch from master to develop April 22, 2018 02:02
@r-khr
Copy link
Contributor Author

r-khr commented Apr 22, 2018

Updated and rebased!

Copy link
Owner

@MrWolfZ MrWolfZ left a comment

Choose a reason for hiding this comment

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

Please fix the issues I pointed out in my comments.

@@ -1,7 +1,7 @@
import { ValidationErrors } from 'ngrx-forms';

/**
* A validation function that requires the value to be non-`null` and non-empty.
* A validation function that requires the value to be non-`undefined` and non-empty.
Copy link
Owner

Choose a reason for hiding this comment

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

Please mention both null and undefined, i.e. "A validation function that requires the value to be non-null, non-undefined, and non-empty."

@@ -23,7 +23,7 @@ import { ValidationErrors } from 'ngrx-forms';
* ```
*/
export function required<T>(value: T | null): ValidationErrors {
Copy link
Owner

Choose a reason for hiding this comment

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

To support strict type checking you need to make it explicit that the function accepts undefined as a value, i.e.

export function required<T>(value: T | null | undefined): ValidationErrors {

This will also fix the TSLint error.

@MrWolfZ
Copy link
Owner

MrWolfZ commented Apr 22, 2018

Thanks for the update. As you can see here there is a TSLint error. Have a look at my inline comments for what needs to be changed.

@r-khr
Copy link
Contributor Author

r-khr commented Apr 22, 2018

Thanks for the review, I updated as requested.

@MrWolfZ MrWolfZ merged commit b5befcc into MrWolfZ:develop Apr 23, 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