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

Refactor Field validation #2169

Closed
alschmiedt opened this issue Dec 13, 2018 · 8 comments
Closed

Refactor Field validation #2169

alschmiedt opened this issue Dec 13, 2018 · 8 comments

Comments

@alschmiedt
Copy link
Contributor

alschmiedt commented Dec 13, 2018

Problem statement

Currently, field.validator_ is called in some of the methods of classes that inherit from Field, but not called in the Field class itself (whether by callValidator(..) or more direct methods). When and where the validators are called in the field classes is not standardized and therefore makes coding and fixing bugs for all field types difficult. It would therefore be more consistent to have the callValidator(..) method called in the Field setValue method.

@AnmAtAnm AnmAtAnm changed the title Refactor callValidator to be in the Field class Refactor Field validation Dec 14, 2018
@RoboErikG
Copy link
Contributor

RoboErikG commented Apr 23, 2019

The base Field now calls the validator as part of the constructor. There is still additional cleanup to fields:

  • Audit fields for cases where the individual field is calling validators instead of the base class.
  • Where appropriate refactor to allow the base class to handle it.
  • As a result of this change, field_number's class validator will to revalidate when changed. It should be updated to re-validate iff the constraints are changed after initializing.

@BeksOmega
Copy link
Collaborator

I ran into this while working on the custom fields demo.

Problem Statement

  1. As mentioned in the OP field validation isn't consistent. This makes it hard to fix bugs.
  2. setValue (in the base class) expects fields to only hold one value. This can be a problem if you're trying to validate something like a vector, a curve, or turtles.
  3. setValue (in the base class) expects the value to be text. This can be a problem if you're trying to validate something like an image (not like a url, like a bitmap), or turtles.

Proposal

Refactor setValue on the base class to be a template that looks something like this:

// What the value contains (be it a string, object, etc) will only be
// known to the developer and the concrete field. To the abstract field
// it will be anonymous.
setValue(value) {
	// Validator defined on the class.
	var newValue = doClassValidation(value);
	// Validator defined by the developer.
	newValue = doUserValidation(newValue);
	// This is where the field stores the validated values however they want
	// (e.g. break an object up into separate variables).
	doValueUpdate(newValue);
	// This makes sure that rerendering is consistent.
	forceRerender();
}

(Plus null checks, sameness checks, events, etc.)

This way the developer could just define the 'do' methods, and know they aren't forgetting anything. And it would be easier to debug because we would know the path would be the same every time.

Also great is that if anyone has overriden setValue this is still backwards compatible because all of the changes would be on the abstract field class.

How does that sound to you peoples?

@rachel-fenichel
Copy link
Collaborator

@NeilFraser do you have any thoughts on this?

@rachel-fenichel
Copy link
Collaborator

I think @BeksOmega's suggestion will work, and it looks like it'll work for backwards compatibility. This is another case where we should have tests that are the same before and after, to verify that existing implementations continue to work.

@BeksOmega
Copy link
Collaborator

Sometimes fields check if they are attached to a block before validating new values, is this the intended behavior?

It looks like this was originally inteded to support headless workspaces? but I think headless workspaces would want their values to be validated as well.

Here's the original commit. Do you remember why this was added @NeilFraser ?

@rachel-fenichel
Copy link
Collaborator

I'm not sure why we do that for text inputs. We do something similar for variables; that's because the set of variables that exists depends on the workspace, and the field doesn't have a pointer to the workspace--but the source block does.

@BeksOmega BeksOmega mentioned this issue May 11, 2019
3 tasks
@BeksOmega
Copy link
Collaborator

Note on this: The field side of this has been fixed, but the events side also needs to be fixed.

For example, if a function-caller-block is notified that the function-definition-block's text input value has changed, and goes to change its label value, the setValue call to change the label will start a new group. This breaks undo.

So the grouping functionality needs to be reworked.

@BeksOmega
Copy link
Collaborator

Closing as the original issue is fixed, and I think the secondary events issue was a misdiagnosis of the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants