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

Garbage input to setter fix #3486

Closed
wants to merge 3 commits into from
Closed

Garbage input to setter fix #3486

wants to merge 3 commits into from

Conversation

mbad0la
Copy link
Contributor

@mbad0la mbad0la commented Oct 6, 2016

Work in Progress

Hi @mj1856 @maggiepint

This PR is in reference to #3483

Please review the current changes and let me know if this hack seems to be doing the trick. Also, let me know of any other steps I need to take to make this PR ready for a merge. Thanks :)

@mattjohnsonpint
Copy link
Contributor

Sorry, this isn't good enough. It needs to actually test the value input parameter of the set function.

@mbad0la
Copy link
Contributor Author

mbad0la commented Oct 11, 2016

So basically, You don't want the setter to set the illegal value and then check whether the instance is valid or not right? You want to prompt at the setter itself, by checking the value?

@maggiepint
Copy link
Member

Correct.

On Mon, Oct 10, 2016 at 8:12 PM, Mayank Badola notifications@github.com
wrote:

So basically, You don't want the setter to set the illegal value and then
check whether the instance is valid or not right? You want to prompt at the
setter itself, by checking the value?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3486 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFxi0iL42FWHjVvEiiHsrxU8V-N-JKf3ks5qyv6AgaJpZM4KQTqo
.

@mbad0la
Copy link
Contributor Author

mbad0la commented Oct 11, 2016

Just a minor doubt. Is there a method in moment to check for the validity of an input as an year, month etc.?

@maggiepint
Copy link
Member

It really need only be numeric. The setters will all overflow into the next
unit if they are out of range. That behavior is expected. So the only thing
that needs validation is that the input is a number, or a string that can
be parsed to a number.

On Mon, Oct 10, 2016 at 10:55 PM, Mayank Badola notifications@github.com
wrote:

Just a minor doubt. Is there a method in moment to check for the validity
of an input as an year, month etc.?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3486 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFxi0n4uqU8u91vTmdeID_66Aoipi6Ahks5qyyS6gaJpZM4KQTqo
.

@mbad0la
Copy link
Contributor Author

mbad0la commented Oct 11, 2016

Hi! I've identified where to change the code. Just need to know how do you need the rejection of illegal input to be handled. Silently ignore it or do something else? Please let me know @mj1856 @maggiepint

Thanks :)

@ichernev
Copy link
Contributor

ichernev commented Nov 1, 2016

@mbad0la like everything else in moment -- we silently ignore errors. If its the constructor then you can make the moment invalid. If its a setter, just ignore the error.

@jsf-clabot
Copy link

jsf-clabot commented Nov 3, 2016

CLA assistant check
All committers have signed the CLA.

@mbad0la
Copy link
Contributor Author

mbad0la commented Nov 9, 2016

@maggiepint

It really need only be numeric. The setters will all overflow into the next
unit if they are out of range. That behavior is expected. So the only thing
that needs validation is that the input is a number, or a string that can
be parsed to a number.

I used this information and added a NaN check on the setter but that has broken the module. It seems the setter does have input types other than numerical. Can you please take a look at it?

@@ -4,7 +4,7 @@ import getParsingFlags from '../create/parsing-flags';
import some from '../utils/some';

export function isValid(m) {
if (m._isValid == null) {
if (m._isValid === null || m._isValid === true) {
Copy link
Member

Choose a reason for hiding this comment

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

This changes validity evaluation from one to continual. I don't see why this change would need to cause that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this change is causing that.

@maggiepint
Copy link
Member

Closing in favor of #3704

@maggiepint maggiepint closed this Jan 8, 2017
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.

None yet

5 participants